This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] implement simple codegen for insertion (and related ops)
ClosedPublic

Authored by aartbik on Oct 14 2022, 5:43 PM.

Details

Summary

This is a proof of concept insertion implementation that sets up
the basic framework and implements it with push backs for just
sparse vectors. It adds insertion/compression through SSA values,
so that we properly update the memref after after pushback operation.

Note that properly using SSA values in sparsification is still TBD
but I will wait until Peiming's loop emitter is in to avoid conflicts.

Diff Detail

Event Timeline

aartbik created this revision.Oct 14 2022, 5:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 5:43 PM
aartbik requested review of this revision.Oct 14 2022, 5:43 PM
aartbik edited the summary of this revision. (Show Details)Oct 17 2022, 2:10 PM
aartbik updated this revision to Diff 468316.Oct 17 2022, 2:11 PM

added SSA values to insert and compress

wrengr accepted this revision.Oct 17 2022, 3:49 PM

A few style nits, but other than that it looks pretty nice

mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
386–389

These are inconsistent re naming the argument and result variables

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
38–39

I'm curious, why is this called "tuple"? Do unrealizedConversionCasts actually return tuples?

280–281

Does ValueRange::operator[] assert/check for out-of-bounds access?

294

why not use rank here since you just asserted they're equal :)

301–303

Why not modify createPushback to store the result into the array it's given, to avoid the code duplication of doing so everywhere it's called?

581–583

Since this idiom shows up a few times, I wonder if we can't use one of the other SmallVector ctors by simply passing tuple.getInputs() and (as necessary) converting that OperandRange to an iterator_range or ArrayRef?

This revision is now accepted and ready to land.Oct 17 2022, 3:49 PM
aartbik marked 6 inline comments as done.Oct 17 2022, 4:59 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
386–389

You mean the %0 and %1. This was really just to introduce some variation in the examples, and avoid naming conflicts with the one at L386. Do you prefer another name here?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
38–39

Ah, the UnrealizedConversionCastOp is really a tuple of values, but without being an explicit tuple. But by using the name and util "tuple" consistently in codegen file, I was hoping to abstract away from the UnrealizedConversionCastOp detail. Now that you mention this, I realize I should also introduce a makeTuple ;-)

280–281

No, added assert for this

301–303

Yeah, good suggestion, not sure why i did not do that in the first place ;-)

aartbik updated this revision to Diff 468375.Oct 17 2022, 5:07 PM
aartbik marked 4 inline comments as done.

addressed comments, added a genTuple util also

wrengr added inline comments.Oct 17 2022, 5:24 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td
386–389

aha. Yeahno, I don't have any preferred names really, just that the inconsistency looked a bit odd. Perhaps you could use %tensor0/%result0 vs %tensor1/%result1?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
581–583

Nice! :)

Though now I have another nit ;) Does genInsert really need to take its arguments as SmallVector/SmallVectorImpl, or could it be adjusted to instead take the arguments directly as OperandRange/ArrayRef/etc? (Albeit, I'm not sure that adjusting genInsert would allow to entirely avoid the intermediate SmallVector— since in various places the SmallVector also gets passed to memref::LoadOp, genEndInsert, genTuple, etc...)

aartbik marked 2 inline comments as done.Oct 17 2022, 5:39 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
581–583

I don't think either of them allows for modifying the contents, as we do now?

This revision was automatically updated to reflect the committed changes.
aartbik marked an inline comment as done.