This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] first version of "truly" dynamic sparse tensors as outputs of kernels
ClosedPublic

Authored by aartbik on Nov 11 2021, 12:00 PM.

Details

Summary

This revision contains all "sparsification" ops and rewriting necessary to support sparse output tensors when the kernel has no reduction (viz. insertions occur in lexicographic order and are "injective"). This will be later generalized to allow reductions too. Also, this first revision only supports sparse 1-d tensors (viz. vectors) as output in the runtime support library. This will be generalized to n-d tensors shortly. But this way, the revision is kept to a manageable size.

Diff Detail

Event Timeline

aartbik created this revision.Nov 11 2021, 12:00 PM
aartbik requested review of this revision.Nov 11 2021, 12:00 PM
aartbik updated this revision to Diff 386653.Nov 11 2021, 1:41 PM

rebased against main branch

aartbik updated this revision to Diff 386670.Nov 11 2021, 2:32 PM

fixed few inconsistencies in comments

aartbik updated this revision to Diff 386675.Nov 11 2021, 3:32 PM

windows build

I still need to take a closer look through the logic of Sparsification.cpp and take a look at sparse_vector_ops.mlir, but other than that things look good modulo my comments

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
246–266

If this op is just for codegen / progressive-bufferization, then shouldn't we know this operand statically whenever we generate the load op? That's certainly the case for Sparsification.cpp:1464 (and conversion.mlir:427, roundtrip.mlir:121, etc). So, are there actually any cases where we need this to be dataflow-dependent, and if not then why not make it an attribute rather than an operand?

258

This description is a bit uninformative, since the reader won't necessarily know which conversions count as "non-trivial" for the purpose of being released. So we should explicate which conversions this includes, either here or at the convert op description (with a comment here that says to look there).

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
876

You're missing an "I"

mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp
293

Add a short description of this out-parameter to the doxygen

544

rename to memTp for consistency with the rest of the code

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
283

Why pointer instead of reference? If pointer is for extern-C reasons, then should assert non-null before dereferencing it

mlir/test/Dialect/SparseTensor/conversion.mlir
417–419

Why the removal of #DenseVector, is that test no longer valid for LoadOp?

mlir/test/Dialect/SparseTensor/roundtrip.mlir
137

reindent/align

aartbik marked 7 inline comments as done.Nov 12 2021, 2:25 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
246–266

Note that it *is* an attribute, but I parse it indeed as an extra operand. So I guess you are proposing to parse the value in a more attribute-like manner ;-)

258

Good point. Added the "non-trivial" definition.

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
876

oh, good catch! we really need to add more "test generators" so we can just iterate over all data types more easily than writing tests by hand ;-)

mlir/lib/ExecutionEngine/SparseTensorUtils.cpp
283

Yeah similar to all the other strided memrefs that are passed between code and lib.
I added the assert to IMPL_LEXINSERT, similar to the other macro tests for non-null.
Good suggestion!

mlir/test/Dialect/SparseTensor/conversion.mlir
417–419

The reconstruct (was .tensor, now .load to be more consistent with tensor equivalent) used to depend on the sparsity structure (so I tested a sparse and a dense one). I opted for the side-effect impureness for the time being, otherwise we get a lot of SSA spaghetti deps in intermediate very short lived code that adds no value. We may refine as needed.

aartbik updated this revision to Diff 386981.Nov 12 2021, 4:19 PM
aartbik marked 4 inline comments as done.

Thanks Wren! Good feedback.

bixia accepted this revision.Nov 15 2021, 2:31 PM
bixia added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
871–876

We have two other places that use similar if-branches, genAddEltCall and genGetNextCall.
Can we avoid duplicating such logic by have getFuncNameSuffix(eltType):

lexInsert+ getFuncNameSuffix(eltType)
This revision is now accepted and ready to land.Nov 15 2021, 2:31 PM
aartbik updated this revision to Diff 387415.Nov 15 2021, 3:02 PM
aartbik marked an inline comment as done.

swap template order

aartbik added inline comments.Nov 15 2021, 3:03 PM
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
871–876

I like that suggestion! Will do in a followup CL