This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Introducing options for the SparseTensorConversion pass
ClosedPublic

Authored by wrengr on Mar 18 2022, 7:59 PM.

Details

Summary

This is work towards: https://github.com/llvm/llvm-project/issues/51652

This differential sets up the options and threads them through everywhere, but doesn't actually use them yet. The differential that finally makes use of them is D122061, which is the final differential in the chain that fixes bug 51652.

Diff Detail

Event Timeline

wrengr created this revision.Mar 18 2022, 7:59 PM
wrengr requested review of this revision.Mar 18 2022, 7:59 PM
wrengr edited the summary of this revision. (Show Details)Mar 21 2022, 1:24 PM
aartbik accepted this revision.Mar 21 2022, 4:17 PM

I have some comments, but okay to go after you address these.

mlir/include/mlir/Dialect/SparseTensor/Transforms/Passes.h
93

Note that in general, I have followed the approach in which we completely separate the "mechanism" (the how to apply a transformation) from "policy" (the when and where to apply, i.e. the heuristics). Here you sort of step into both worlds where kAuto is entering the policy world, while the kVia/kDirect are still just two mechanisms. So I would be okay with just the two mechamisms for now (although you explained offline why kAuto is there). If we keep all three, perhaps mention a bit on policy vs mechanism (in general in this file perhaps at top, and concretely here).

And yes, I know, I am deviating from my regular "terse" commenting path ;-) ;-)

96

Good to add this reference, but perhaps push it to the actual implementation part, since there is not much background needed for the Via vs Direct enum here

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

is the underscore name really neeed? can't we just call it tpConv or so if you avoid a name clash?

This revision is now accepted and ready to land.Mar 21 2022, 4:17 PM
wrengr updated this revision to Diff 417158.Mar 21 2022, 7:01 PM

Adding commentary about separating mechanism vs policy.

wrengr marked an inline comment as done.Mar 21 2022, 7:02 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/SparseTensor/Transforms/Passes.h
96

I added the reference just to give a bit more explanation about what "direct" means, since there are many possible options for direct conversion in the literal/colloquial sense (e.g., the AMT approach I mentioned offline). Honestly, I don't really like the name "direct" because of this ambiguity; but then I couldn't think of a more explicit name (other than something like alaTACO, which also isn't great).

...Maybe a name like kViaNNZ would be better, since it uses the SparseTensorNNZ class as the intermediate? (Not that I'm especially fond of the name of that class either.) That would make room for adding kViaAMT (etc) down the road

wrengr updated this revision to Diff 417161.Mar 21 2022, 7:04 PM
wrengr marked an inline comment as done.

renaming variable to avoid underscore