This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] change variable dimension to fixed attribute pointers/indices
ClosedPublic

Authored by aartbik on Sep 7 2022, 3:22 PM.

Details

Summary

The "sparsification" pass does not need the ability to use runtime values for
the dimension, so the only source for variability would have been user code.
Restricting the dimension to constants simplifies code generation.

Diff Detail

Event Timeline

aartbik created this revision.Sep 7 2022, 3:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 3:22 PM
aartbik requested review of this revision.Sep 7 2022, 3:22 PM
Peiming accepted this revision.Sep 7 2022, 3:26 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp
486

Much more clear!

This revision is now accepted and ready to land.Sep 7 2022, 3:26 PM
wrengr accepted this revision.Sep 7 2022, 4:05 PM

Very nice! Just recently I was wondering to myself why we supported SSA-value dimensions here when we don't for the various other ops (e.g., concat). This helps clean things up a lot :)

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

I can't help but wonder if we shouldn't just define a helper method to do this, since we use that particular expression in a number of different places. Perhaps even just pass getDimension an implicit bool for whether to do zero-extension or not?

aartbik updated this revision to Diff 458592.Sep 7 2022, 4:09 PM

proper sign in comparison

aartbik marked an inline comment as done.Sep 7 2022, 4:12 PM
aartbik added inline comments.
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorConversion.cpp
1081

Agreed, sounds like a good idea (in follow up revision) to move this into a shared util place

This revision was landed with ongoing or failed builds.Sep 7 2022, 4:27 PM
This revision was automatically updated to reflect the committed changes.
aartbik marked an inline comment as done.