This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] implement sparse tensor init operation
ClosedPublic

Authored by aartbik on Oct 13 2021, 6:35 PM.

Details

Summary

Next step towards supporting sparse tensors outputs.
Also some minor refactoring of enum constants as well
as replacing tensor arguments with proper buffer arguments
(latter is required for more general sizes arguments for
the sparse_tensor.init operation, as well as more general
spares_tensor.convert operations later)

Diff Detail

Event Timeline

aartbik created this revision.Oct 13 2021, 6:35 PM
aartbik requested review of this revision.Oct 13 2021, 6:35 PM
aartbik updated this revision to Diff 379581.Oct 13 2021, 7:00 PM

rebased with main

Overall I like the changes in SparseTensorConversion.cpp, but I'm a little worried about switching from APInt to attributes because of the comment about blowing up compiler memory usage in mlir/lib/Dialect/Linalg/Transforms/ElementwiseOpFusion.cpp on lines 1380–1382. Do we have anything set up to track performance metrics and prevent regressions?

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

Typo: ".." -> "."

148

This should be called genAlloca (or similar) since it generates an alloca rather than a malloc

178–184

Not sure if that'll allow the last statement to fit on one line, but it's a cleanup I've been meaning to make for a while. Feel free to choose a better name for dlt of course.

wrengr added inline comments.Oct 14 2021, 12:48 PM
mlir/lib/ExecutionEngine/SparseUtils.cpp
186

Shouldn't that just be "per-dimension"?

193–196

Throughout this file, I feel like the old d name makes more sense, since we're iterating through dimensions d1,d2,...dr up to the rank r.

197–201

Given that we keep calling this for all the loops, it should be floated out to the top of the function, to reduce boilerplate

270

Please rename this to rank, to avoid confusion with the sizes parameter and for consistency with the rest of the file

543

+1 :)

conversion.mlir looks at least conceptually good to me, though I'm not familiar enough with FileCheck to sign off on it.

The other three files LGTM once the comments are addressed

aartbik updated this revision to Diff 379847.Oct 14 2021, 2:33 PM
aartbik marked 7 inline comments as done.

addressed Wreng's comments, thanks!

wrengr accepted this revision.Oct 14 2021, 5:51 PM
This revision is now accepted and ready to land.Oct 14 2021, 5:51 PM
This revision was automatically updated to reflect the committed changes.