Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse CPU integration tests are leaking #51388

Closed
joker-eph opened this issue Oct 3, 2021 · 8 comments
Closed

Sparse CPU integration tests are leaking #51388

joker-eph opened this issue Oct 3, 2021 · 8 comments
Assignees
Labels
bugzilla Issues migrated from bugzilla mlir:core MLIR Core Infrastructure

Comments

@joker-eph
Copy link
Collaborator

Bugzilla Link 52046
Resolution FIXED
Resolved on Oct 05, 2021 10:02
Version unspecified
OS All
CC @sherhut,@jpienaar,@River707

Extended Description

Running with ASAN, tests like Integration/Dialect/SparseTensor/CPU/sparse_sum.mlir have multiple leaks with memory is allocated from _mlir_ciface_newSparseTensor.

I disabled the leak checker for now in mlir/test/Integration/Dialect/SparseTensor/CPU/lit.local.cfg

@joker-eph
Copy link
Collaborator Author

assigned to @aartbik

@joker-eph
Copy link
Collaborator Author

The Sparse tensor dialect has a "new" operation but nothing to "destroy" apparently?

https://mlir.llvm.org/docs/Dialects/SparseTensorOps/#sparse_tensornew-mlirsparse_tensornewop

@aartbik
Copy link
Contributor

aartbik commented Oct 3, 2021

Yes, the memory management was still a bit "in flight" waiting for a full bufferization story, but I agree that does not mean we should not have a proper delete available (the runtime support it, there is just no "delete" op yet).

I will add that Monday, so we can start using the leaker properly. Thanks for bringing this to higher priority.

@sherhut
Copy link

sherhut commented Oct 4, 2021

With the recent changes to the buffer deallocation pass, it should now also be able to handle your use case. You will need to implement the AllocationOpInterface for you new operation so that the pass knows how to deallocate and clone a sparse tensor.

The clone operation is only needed if you have control flow.

@joker-eph
Copy link
Collaborator Author

Note that in this case these are .mlir tests that are written with the leaks, it does not come from a bufferization pass introducing leaking allocations.
I just couldn't update the test without a primitive available for the "dealloc".

@aartbik
Copy link
Contributor

aartbik commented Oct 4, 2021

Yes, ad Stephan's coomment, this is not the "inplaceable" example I usually present as something that needs to be addressed.

I was merely referring to the fact that having "tensors" as type and "sparse storage schemes" as underlying storage formats creates some tension on new/delete operations. But I found a good compromise for now which will fix the leaks.

I expect to refine the ops as our bufferization strategy at-large converges on a single solution for the dense and sparse world.

@sherhut
Copy link

sherhut commented Oct 5, 2021

The buffer-deallocation pass can be run once the program already is on buffers. It does not reason about performing computations in place. It only manages lifetimes by inserting appropriate dealloc operations.

For hand-written tests this is not required but might be useful in an end to end flow.

@aartbik
Copy link
Contributor

aartbik commented Oct 5, 2021

Revision https://reviews.llvm.org/D111099 addresses the immediate concern of leaking memory. The sparse integration tests can now run with asan enabled again, since all memory is properly released.

In the long run, we need to unify dense and sparse bufferization and memory management somehow. Mehdi's efforts to have asan on by default have put this strongly on my immediate radar screen!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla mlir:core MLIR Core Infrastructure
Projects
None yet
Development

No branches or pull requests

3 participants