Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp | ||
---|---|---|
526 | while you are here: that do not alter (not does) | |
536–537 | can we make this / an and | |
536–537 | I would continue this comment on the same line L585, since it just continues the reasoning | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
362 | Generates code | |
367 | Generates code | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
98 | put this before the if (so that First, Second, Third are at same leve) | |
111 | put this Third before the if | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.h | ||
193 | Generates | |
272 | related | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
104 | an attribute | |
111 | directly return in fact, write this as if (Value v = ...) return v ... | |
696 | xxx -> op? | |
1090–1094 | 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 | ||
---|---|---|
600 | ditto | |
610 | ditto | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
365 | If this is actually a dimension, then use the Dimension type. (Otherwise, should rename the variable) | |
370 | If this is actually a dimension, then use the Dimension type. (Otherwise, should rename the variable) | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
47 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
53 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
62 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
71–75 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
84 | If this is actually a level, then use the Level type. (Otherwise, should rename the variable) | |
91 | 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 | ||
106 | I'm pretty sure there's already something that does this in StaticValueUtils.h |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
604 | 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 ... | |
614 | ditto | |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
49 | You should add a FIXME comment about how toOrigDim is deprecated. | |
55 | ditto | |
120 | 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 | ||
1072 | 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 | |
1085–1086 | yes please :) |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
---|---|---|
91 | it means transformed coordinates. | |
120 | 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 | ||
106 | Thanks for letting me know. |
mlir/lib/Dialect/SparseTensor/Transforms/LoopEmitter.cpp | ||
---|---|---|
120 | 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. | |
238–244 | Elsewhere the code always uses the name "pHi", so that name should be used here too | |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
1072 | 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 | ||
395 | 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 | ||
---|---|---|
238–244 | 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)