Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
---|---|---|
500 | while you are here: that do not alter (not does) | |
510–511 | can we make this / an and | |
510–511 | I would continue this comment on the same line L585, since it just continues the reasoning | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
342 | Generates code | |
347 | Generates code | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
93 | put this before the if (so that First, Second, Third are at same leve) | |
106 | put this Third before the if | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h | ||
193 | Generates | |
274 | related | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
103 | an attribute | |
110 | directly return in fact, write this as if (Value v = ...) return v ... | |
670 | xxx -> op? | |
1067–1071 | we cannot generate |
A bunch of repetitive comments about using the proper types. If you rebase this after D145756, then that handles a bunch of them.
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
561 | ditto | |
571 | ditto | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
345 | If this is actually a dimension, then use the Dimension type. (Otherwise, should rename the variable) | |
350 | If this is actually a dimension, then use the Dimension type. (Otherwise, should rename the variable) | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
46 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
52 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
59 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
66–70 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
79 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
86 | I've noticed you use this variable name a lot in this file. What exactly do you mean by it? Seems like there should be a more informative name | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h | ||
197 | Is this actually (1) a memref of all the coordinates associated with a single element; in which case it should be "coords"; or (2) a single coordinate value; in which case it should be "crd"? | |
198 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
105 | I'm pretty sure there's already something that does this in StaticValueUtils.h |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
565 | This should just check the offset local variable, rather than calling the function again. And for brevity it ought to use the syntax if (auto offset = ...) return ... | |
575 | ditto | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
48 | You should add a FIXME comment about how toOrigDim is deprecated. | |
54 | ditto | |
115 | Should just use a foreach loop instead; i.e., for (auto cond : ArrayRef(conds).drop_front()) | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h | ||
193 | "are" | |
194 | "in" | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
1049 | Instead of doing this manually, it would be a lot easier to follow if you use for (... : llvm::enumerate(llvm::zip(...))) instead Also, should use the Dimension type | |
1062–1063 | yes please :) |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
---|---|---|
86 | it means transformed coordinates. | |
115 | conds is an SmallVector, and it does not provide a drop_front, I prefer to start with 1 here instead of casting it to ArrayRef before iteration. | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h | ||
198 | The header for Level is not included yet, will do it later to avoid more conflicts. | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
105 | Thanks for letting me know. |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
---|---|---|
115 | I still think the foreach loop is cleaner, since it captures the intention directly and avoids introducing extraneous details like the index variable and getting the size. | |
233–239 | Elsewhere the code always uses the name "pHi", so that name should be used here too | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
1049 | Elsewhere throughout the MLIR code folks name this "it". Also, you should make it auto& to avoid an unnecessary copy | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorStorageLayout.h | ||
344 | Did you mean to add this back in? Back when I renamed stuff in this file I cleaned all those up (since at that point, at least, the StorageSpecifierKind argument always took a Level), so the "[CLARIFY_DIM_LVL]" note no longer exists. If this CL reintroduces ambiguity/confusion issues, then you should give a longer comment explaining what the problem is |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
---|---|---|
233–239 | hmm, but it comes from createOrFoldDimOp(d) so shouldn't it be dimSz? Then again, since it's being stored into lvlSizes, that does suggest it ought to be lvlSz... So can you add a comment explaining why createOrFoldDimOp(d) is safe/correct to use here? (And presumably it'll be a TODO/FIXME, since I'm guessing this is just a stand-in for once I finish up the proper dim<->lvl conversion class) |
while you are here:
that do not alter (not does)