This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Make sparse_tensor::StorageLayout publicly available.
ClosedPublic

Authored by Peiming on May 16 2023, 4:52 PM.

Diff Detail

Event Timeline

Peiming created this revision.May 16 2023, 4:52 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Peiming requested review of this revision.May 16 2023, 4:52 PM
Peiming updated this revision to Diff 522848.May 16 2023, 5:03 PM

revert unintended changes.

Peiming updated this revision to Diff 522849.May 16 2023, 5:05 PM

update bazel file

Peiming updated this revision to Diff 522850.May 16 2023, 5:07 PM

minor fixes.

Peiming updated this revision to Diff 522852.May 16 2023, 5:11 PM

remove outdated files.

wrengr added inline comments.May 17 2023, 6:31 PM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorStorageLayout.h
13

This macro should match the name of the file

22

Since this whole section (lines 22–82) is intended as documentation, it should use "///" in lieu of "//".

23

Although this probably unchanged from before, this modifier is very strange. I'd suggest dropping it (without adding anything in its place).

84–96

Instead of doing these static-assertions, it'd be a lot simpler and clearer to define the SparseTensorFieldKind elements directly in terms of their corresponding StorageSpecifierKind elements. I've attached a change which demonstrates what I mean.

I didn't adjust the definition of SparseTensorFieldKind::StorageSpec since it didn't have a corresponding static_assert; but you can always repeat the pattern if you wanted it to match StorageSpecifierKind::LvlSize. (And if you're paranoid about the possibility of the StorageSpec clashing with the other elements, then you can always add static asserts to check that.)

120

it'd be better to use const STT& (since STT doesn't use the pimpl idiom).

121

afaict there's no benefit to marking this parameter const (since STEA uses the pimpl idiom)

153

This defn should be moved to the cpp file, rather than being defined in the class (since it's too big to benefit from being inlined)

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

I think it would be better to move this whole section into a separate "SparseTensorStorageLayout.cpp" file, rather than putting it here. (Fwiw, in my work on updating STEA to use the new syntax, I'm also planning on splitting all the methods for that type off into their own cpp file as well)

49

this should be static constexpr

55

this should be "lvl"

56

ditto

59

be sure to rebase this CL over D150330 (and D150822, D150830)

64

should be "level"

66

This sentence is unnecessary (since level-types are always in the same order as the levels themselves) and confusing (since it does not match the new "dimension"/"level" terminology).

70–73

I think it would be a lot cleaner to define new helper predicates: constexpr bool hasPosDLT(dlt) { return isCompressedDLT(dlt) || isCompressedWithHiDLT(dlt); } and constexpr bool hasCrdDLT(dlt) { return isCompressedDLT(dlt) || isCompressedWithHiDLT(dlt) || isSingletonDLT(dlt); } and then use those here instead. Especially because those predicates will also be helpful elsewhere as well.

80

You should define static constexpr Level kInvalidLevel = -1u; and use that here, instead of using the literal -1u.

84

ditto

101

Should be "specType" or "specifierType" (since we renamed the StorageSpecifierType)

145

should be "specifier", right? Also, isn't the specifier actually the first field rather than the last?

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorDescriptor.h
65

Since you have a StorageLayout ctor which takes the STT directly (and calls getEncoding itself), you should just use "layout(stt)" here

66

You should have the StorageLayout ctor assert that the STEA isn't null, instead of doing this assertion here.

Peiming marked 21 inline comments as done.May 17 2023, 9:01 PM
Peiming added inline comments.
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
46

I think a better way is to keep it in SparseTensorDialect.cpp, but moving the other ops-related part into SparseTensorOps.cpp as other dialects in MLIR.

Either way, I will probably keep it this way for this revision.

70–73

I will address this in a separate patch.

145

No, the specifier is the last field.

Peiming updated this revision to Diff 523252.May 17 2023, 9:02 PM
Peiming marked 3 inline comments as done.

rebase and address comments.

aartbik added inline comments.May 18 2023, 10:24 AM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorStorageLayout.h
29

I think this is the fourth time we moved this description around ;-)

135

since you document every getter except the first one, you might as well get rid of this "section header" and add doc to L137

157

Here it seems wortwhile to add a "section header" and say that some method (inline or prototype) follow


Storage related methods.
//

or something like that

179

extra line before closing namespaces

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

don't use the sparse_tensor:: qualifier here to be consistent with all the other header docs in this file

46

Yeah, right now we have one SparseTensorDialect.cpp to "rule them all" and the definitely makes things simpler (getting all the deps right for example).
But the file did grow in size over time.

Something to think about for the future, but I am okay with the current placement

mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorDescriptor.h
1

this top level comment is now out of date

13

so is this macro (seems the mistake goes even one generation back?)

Peiming updated this revision to Diff 523443.May 18 2023, 10:35 AM
Peiming marked 7 inline comments as done.

address comments.

aartbik accepted this revision.May 18 2023, 10:37 AM
This revision is now accepted and ready to land.May 18 2023, 10:37 AM
wrengr added inline comments.May 18 2023, 11:47 AM
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorStorageLayout.h
114–116

It's cleaner to delegate to the other ctor.

134

"Gets"

wrengr added inline comments.May 18 2023, 11:57 AM
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp
151

Since FieldIndex isn't Level, I don't think you should use kInvalidLevel here. So I guess you need to define kInvalidField too.

Peiming updated this revision to Diff 523519.May 18 2023, 12:51 PM
Peiming marked 2 inline comments as done.

address comments.

Peiming marked an inline comment as done.May 18 2023, 12:55 PM
This revision was landed with ongoing or failed builds.May 18 2023, 1:29 PM
This revision was automatically updated to reflect the committed changes.