The indices for insert/compress were previously provided as
a memref<?xindex> with proper rank, since that matched the
argument for the runtime support libary better. However, with
proper codegen coming, providing the indices as SSA values
is much cleaner. This also brings the sparse_tensor.insert
closer to unification with tensor.insert, planned in the
longer run.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Note that some singleton stuff sneaked into the sparsification file, but I will rebase that as soon as it goes in, so the diff will not show that anymore (but it seemed a bit pointless to refactor it out right now....)
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
233 | love it! | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
609–610 | Is it intended? It should be redundant now, sine you do not need to stored the index? Or are you foreseeing that it will be needed in the near future? | |
810–813 | Is it always true that the first rank loop always corresponds to output tensor? (Of course, without considering affine) |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
383 | nit: remove {} when there is only one statement (finally my turn ;-) |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
233 | Me too, it is very readable this way. Bugs are easier to find! | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
383 | :-) But here, the true=branch has braces and then the else branch must follow. | |
609–610 | Oh, yeah, I left it here so that it is clear nothing is done for sparse output. else if (t != codegen.sparseOut) { // Annotated sparse tensors (not involved in output) do you like that better? | |
810–813 | Yes, due to our topological sort *and* the criteria for expansion. No sparse tensor is ever visited non-lexicographical, and we only accept expansion for parallel outer , non related loops inner |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
609–610 | Yeah, look strange when you have empty branch. Or // Sparse out (do nothing) |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
228 | should surround with backticks (or single quotes?) | |
326 | Does using single quotes cause this to make a link? or should these be backticks? | |
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
456–468 | Can these be implemented via another TypesMatchWith trait (or similar) in the td-file, rather than with a separate verifier? | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
1231 | why using the op's tensor rather than the adaptor's tensor? Unless that's necessary for correctness, it would be cleaner to reuse the tensor variable bound on the previous line |
Thanks for the feedback!
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
228 | Yeah, good suggestion | |
326 | backticks | |
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
456–468 | Looks like both need to be custom checks right (note that other stuff is done through traits, but these check the length of indices, +1) | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp | ||
1231 | This is the correct idiom, since we need the original type. Note that we use adaptor.getXXX for rewriting, but when querying before-conversion types (or attributes), using the op is acceptable idiom (per Alex and River even ;-) |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
---|---|---|
1900 | Ha! Not sure why I changed that ;-) |
should surround with backticks (or single quotes?)