This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] factored out a new DimLvlMapping class
AbandonedPublic

Authored by wrengr on Jan 17 2023, 4:19 PM.

Details

Summary

The new class generalizes dim<->lvl conversion logic, so that it can be reused for both the runtime and codegen paths. This abstraction is also the first step towards supporting non-trivial non-permutations.

Diff Detail

Event Timeline

wrengr created this revision.Jan 17 2023, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2023, 4:19 PM
wrengr requested review of this revision.Jan 17 2023, 4:19 PM
Peiming added inline comments.Jan 17 2023, 5:26 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
196

Do we really need the cache? It does not seem like a complicated operation anyway, plus it might make the class expensive to copy.

305

I think requires user to pass in a loc on every call is more flexible, maybe the location need to be updated between calls (maybe builder as well).

Peiming added inline comments.Jan 17 2023, 5:28 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
238

You could probably avoid the virtual function here using CTRP

Peiming added inline comments.Jan 17 2023, 5:30 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
193–194

Why not call enc.getxxx for these two? Should be a cheap direct field queries.

wrengr added inline comments.Jan 17 2023, 5:35 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
196

At the moment the cache is trivial, because we're only handling permutations; but that'll change once I add the code for handling non-permutations. Considering how aggressively everything else in MLIR memoizes stuff, I don't think doing this is out of place. If/when we start needing to pass these around, then we can split the class into class DimLvlMappingImpl {...} vs class DimLvlMapping { DimLvlMappingImpl impl; } like is done everywhere else in MLIR.

305

So far all our code is structured to reuse the same loc/builder everywhere, so it's a lot cleaner to just pass them into the ctor rather than passing them into every method call.

wrengr added inline comments.Jan 17 2023, 5:44 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
193–194

Take a look at the ctor. These fields are memoized to avoid repeating the conditionals there, rather than to avoid repeating the getter methods.

238

Yeah, we could use CRTP instead. Personally I prefer virtual methods since they give much better error messages when things go wrong, but I'm not sure how bad the performance cost is for this case.

wrengr updated this revision to Diff 490686.Jan 19 2023, 4:32 PM

Updating NewCallParams::genBuffers to take a reference rather than copying the DimLvlMapping

aartbik added inline comments.Jan 27 2023, 12:35 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
127

Why this change? Is it possible to have no internal Attribute implementation?

mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
70

This really is a wrapper around a SparseTensorEncodingAttr and provides a consistent terminology and look into these. As such, do you think SparseTensor/IR is a better place for this (with "public" header). The codegen class below clearly belongs here, but I am wondering if moving this to a general place will enable a lot more simplification in the long run.

Otherwise, it seems a very heavy abstraction for "just" codeegen....

wrengr added inline comments.Jan 27 2023, 3:35 PM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
127

Yes, it's possible not to have the impl (e.g., when the Attribute was build via the default-ctor).

The operational justification for making this change is because calling env.hasIdDimOrdering() will call env.getDimOrdering() which will call env.getImpl()->dimOrdering — which means that calling hasIdDimOrdering on the null-attribute will end up calling operator-> on the nullptr, which will crash.

The semantic justification for making this change is because we interpret dense-tensors (aka tensors will null-STEA) in the same way as sparse-tensors with the default STEA. And since the default dimOrdering field is the identity, thus we interpret dense-tensors as having the identity ordering as well.

My original motivation for making the change was so that we can just say enc.hasIdDimOrdering() in the DimLvlMapping ctor, rather than needing to phrase the check as (!enc || enc.hasIdDimOrdering()). [Back when I started this CL the hasIdDimOrdering method wasn't around yet, so I just had a standalone function that performs the same implementation as I've given the method in this version of the CL.] However, since making the change I've noticed there are several other places in the code that use the latter expression, which could now also be cleaned up to simply call the method.

mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
70

It's more a wrapper around RankedTensorType, since it includes information about the shape as well as the encoding. But yes, I agree.

Fwiw, I've been working on a rewrite of this CL since the currently posted version doesn't quite work out for being easily reused by the codegen pass (with the way the codegen pass is currently structured). And in that rewrite I've been moving this class towards being more and more of just a wrapper around RankedTensorType / ShapedType+STEA. In particular I've removed the stuff for memoizing and querying the levels, since the only places that actually need that are places that need the full DimLvlBuilder versions instead.

Once I get that variation finalized, I'd be happy to move it to the IR folder and make it public. One of the stumbling blocks I've been having, though, is what exactly to rename it to. My initial unfiltered thought would be to name it SparseTensorType (because of it's original motivation); but that's not a good name since it's specifically designed to abstract over the differences between sparse- and dense-tensor types, whereas that name seems to suggest that it's specifically for sparse-tensors alone. I suppose I could go for something like SparseLikeTensorType or SparseTensorAdaptorType etc, but those are a mouthful and also all feel a bit off... thoughts?

wrengr added inline comments.Jan 27 2023, 3:55 PM
mlir/lib/Dialect/SparseTensor/Transforms/DimLvlMapping.h
70

Yeahno, after tossing it about a bit more, I'm thinking SparseTensorType might be the best name —since it does things like renaming the various methods to make clear whether they're talking about dimensions vs levels. I'll just be sure to add some documentation clarifying the fact that it can and should be used for both dense- and sparse-tensors per se.

wrengr abandoned this revision.Feb 15 2023, 11:56 AM

This patch has been redesigned and replaced by other patches (D143800, D143946, D143949, D144052, et seq.)