This is a very careful start with alllowing sparse tensors at the
left-hand-side of tensor index expressions (viz. sparse output).
Note that there is a subtle difference between non-annotated tensors
(dense, remain n-dim, handled by classic bufferization) and all-dense
annotated "sparse" tensors (linearized to 1-dim without overhead
storage, bufferized by sparse compiler, backed by runtime support library).
This revision gently introduces some new IR to facilitate annotated outputs,
to be generalized to truly sparse tensors in the future.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Some initial questions. The code looks good to me, though my code review muscles are out of shape, so I will take another pass after these comments!
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
125 | Question for my own understanding: later, on line 248, you assert that there's just a single input argument (if I understand the code correctly). Will this always be the case? If so, why is this Variadic? Is ins always expected to be variadic? | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
285 | Just to confirm: is this rewrite effectively a no-op? It just replaces the operator with the underlying pointer? | |
mlir/test/Dialect/SparseTensor/dense.mlir | ||
118 | Where would this fail, and why, exactly? |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
125 | Good question. This is forward looking to sparse outputs, where we will need to feed in all arrays that constitute the sparse tensor (see L 146 for an example of that). All this is pretty much moot with the current approach of lowering to runtime support library (where the operation is folded again into the opaque pointer) but alternative implementations are possible in the future (e.g. actual code generation for stuff that is currently backed by the runtime support library). In such cases, we must correctly reflect the fact that reconstructing a tensor from sparse storage schemes needs all arrays, even though currently it doesn't. | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
285 | Yes, even the variadic (in a later revision) will just verify that they all map to the same pointer and lower it to the no-op. However, if we abandon the runtime support backing with actual codegen, that may chane in the future, and it is better to correctly reflect the full depenence on the sparse storage scheme! | |
mlir/test/Dialect/SparseTensor/dense.mlir | ||
118 | L623 in Sparsification.cpp Not having the same "buffer" going in and out would require an explicit memory allocation for the tensor into which we reconstruct the sparse tensor according to the op (see the two ways of inplace/not-in-place for non-annotated outputs). |
After these comments, I think I'm good! However as I said, someone else may want to review this, as I'm brand new to this code and frankly still do not understand everything that is happening.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
125 | Ah, I see! I didn't think about how you'd potentially need multiple memrefs to implement a sparse tensor. | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
381–385 | ||
750 | Should this comment be changed? Seems like this supports sparse tensors as well now? |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
750 | Good catch. Changed. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
---|---|---|
278 | s/fold/folds/ | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
730–742 | It seems to me that this code block calculating the args for load is repeated for store. If I am right here, we should find a way to put this to a subroutine for reuse. |
Thanks all!
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
730–742 | Yes, with this move towards allowing sparse stores, the code is much more similar to before. I kept it like this to make the diffs smaller, but I will unify in next CL that touches this! |
Question for my own understanding: later, on line 248, you assert that there's just a single input argument (if I understand the code correctly). Will this always be the case? If so, why is this Variadic? Is ins always expected to be variadic?