Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
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. |
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. |
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
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). 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?) |
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. |
This macro should match the name of the file