This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Combining `dimOrdering`+`higherOrdering` fields into `dimToLvl`
ClosedPublic

Authored by wrengr on May 25 2023, 2:51 PM.

Details

Summary

This is a major step along the way towards the new STEA design. While a great deal of this patch is simple renaming, there are several significant changes as well. I've done my best to ensure that this patch retains the previous behavior and error-conditions, even though those are at odds with the eventual intended semantics of the dimToLvl mapping. Since the majority of the compiler does not yet support non-permutations, I've also added explicit assertions in places that previously had implicitly assumed it was dealing with permutations.

Diff Detail

Event Timeline

wrengr created this revision.May 25 2023, 2:51 PM
Herald added a project: Restricted Project. · View Herald Transcript
wrengr requested review of this revision.May 25 2023, 2:51 PM
wrengr updated this revision to Diff 525829.May 25 2023, 2:53 PM

git-clang-format

wrengr updated this revision to Diff 525881.May 25 2023, 4:50 PM

updating the STEA-documentation examples

aartbik added inline comments.May 25 2023, 5:41 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
170

even though this will be corrected by your follow up improvements, it may be worthwhile to note that the level types are applied after the dim to level translation (your improved renaming went a long way to make this more clear, but e.g. make it clear that (i,j)->(j,i) and dense, compressed is really CSC since dense applied to outer j

I thought we had that somewhere, but can't find it

wrengr updated this revision to Diff 526168.May 26 2023, 1:02 PM

adding documentation for the dim/lvl distinction and how the various fields correspond

wrengr marked an inline comment as done.May 26 2023, 1:03 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
170

I added a brief discussion (further up, before describing the fields) of the dim/lvl distinction and how the fields correspond. Let me know if that's sufficient.

Fwiw, I was hoping to avoid doing too much rewriting of the STEA documentation prior to introducing the new syntax; just to avoid spending too much time on editing and rebasing churn. I really like the StableHLO documentation we wrote up, and was planning to do a wholesale replacement of the current STEA documentation with that StableHLO documentation once I get the new parser/syntax landed.

aartbik accepted this revision.May 26 2023, 1:27 PM
aartbik added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
170

Yeah, I am not asking for any more rewriting at this point, no worries. This just jumped out a bit (but even then, I agree it was a pre-existing lack of explanation, not really related this changed).

mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
551

SGTM

This revision is now accepted and ready to land.May 26 2023, 1:27 PM
wrengr updated this revision to Diff 526204.May 26 2023, 2:57 PM
wrengr marked an inline comment as done.

rebase

Peiming added inline comments.May 30 2023, 1:49 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
128–142

This is NICE!

238–239

How does compressed deal with potential trailing zeros in ELL?

wrengr marked an inline comment as done.May 30 2023, 3:19 PM
wrengr added inline comments.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td
238–239

This example for ELL is out of sync with how we'll actually be handling ELL going forward. For this commit I'm just doing the minimal changes necessary to keep things consistent. When I finish the commit for using the new syntax, I'll update the example to match the actual design we'll be using

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