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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
154 | 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 |
adding documentation for the dim/lvl distinction and how the various fields correspond
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
154 | 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. |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
154 | 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 |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
222–223 | 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 is NICE!