Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/SparseTensor/IR/CMakeLists.txt | ||
---|---|---|
7–8 | Since we already have the MLIRSparseTensorEnums library, you should rename these to something else to avoid confusion. Elsewhere in MLIR they often just append "enums" to the name of the file containing the definitions (e.g., SparseTensorAttrDefsEnums.{h,cpp}.inc; though I'd shorten that to SparseTensorAttrEnums.{h,cpp}.inc since that's more legible) |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
181–184 | Elsewhere we've been moving away from the "k" prefix on enum ctors. Do we really want the "k" here or no? | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
186–187 | perhaps rephrase this to something more like "holds the sizes for dimensions, pointer arrays, index arrays, value arrays. This includes the buffer memory for storing those sizes, but not the buffer memory for the tensor itself." Though I'm not sure if the second sentence is really necessary (since imo it's implied by the preceding sentence). |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
---|---|---|
208 | grammatically that should be "held by". Though I think it'd be better to say "Returns the requested field of the metadata." | |
212 | "size of the" | |
212 | This should be "level" rather than "dimension", and ditto for most (if not all) of the other places in this CL. The one place I can think of where "dimension" is correct is for some of the uses of kDimSize. However, we will actually want to have both kDimSize and kLvlSize. See D139165 for more details about why we want both. | |
234 | Grammatically that should be "Sets the requested field of". Though semantically it'd be better to rephrase the sentence as "Sets the field of the metadata to the given value." | |
239 | "level". | |
239 | "size of the" |
Also it looks like you forgot to commit the contents of mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.h
replace "sparse_tensor.storage_specifier" with "sparse_tensor.storage_specifier.init"
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h | ||
---|---|---|
23 | if you prefer, you can avoid all of this by simply repeating the define #define GET_ATTRDEF_CLASSES #define GET_ATTRDEF_CLASSES | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
16–20 | Introduce a new section here, and below for the new stuff just so we split the file a bit into parts ===----------------------------------------------------------------------=== | |
178 | period at end | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td | ||
33 | is this still in the making? | |
85 | newline? | |
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
344 | can we move this to the top with the other #define/include parts above? that way we have less includes spread out |
address comments from Aart.
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensor.h | ||
---|---|---|
23 | I actually learned the trick for IREE through code search. But yeah, your way seems much more clear. Updated! | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td | ||
33 | Overlooked it... Now complete | |
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
344 | Okay. make sense! |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorAttrDefs.td | ||
---|---|---|
182 | still missin period after kind ;-) | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorOps.td | ||
184 | Return an initial .... Also, I think initial can use some explanation here | |
185 | index arrays, and (Oxford comma) | |
208 | In all other ops, we either just say Example, or add explanation to the "header". Example of querying the size .... would be more consistent? | |
235 | same suggestion, combine with Example header | |
mlir/include/mlir/Dialect/SparseTensor/IR/SparseTensorTypes.td | ||
25 | wait, this is not Sparse Tensor Types, but Storage Specifiers right? Different header? Although we have a bit of a misnomer filename (SparseTensorTypes, should that be StorageSpecifierTypes? |
Since we already have the MLIRSparseTensorEnums library, you should rename these to something else to avoid confusion. Elsewhere in MLIR they often just append "enums" to the name of the file containing the definitions (e.g., SparseTensorAttrDefsEnums.{h,cpp}.inc; though I'd shorten that to SparseTensorAttrEnums.{h,cpp}.inc since that's more legible)