Details
- Reviewers
aartbik nicolasvasilache bixia wrengr anlunx
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
173–179 | This whole section is very confusing due to the variable names. Is dimType supposed to be a DimLevelType? If so, then why do you use is{Compressed,Singleton}Dim instead of is{Compressed,Singleton}DLT? and why is the field called dims rather than something like dimLevelTypes which would make it clear? If not, then what is dimType supposed to be? | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
297 | I don't feel like this class really belongs in this file— since all the other things in CodegenUtils.{h,cpp} are small generic helpers, whereas this class is both rather large and pretty specific. Why not split it out into a separate LoopEmitter.{h,cpp} file? | |
324–335 | Why have this as an inline definition rather than moving it to the cpp file like most of the other methods? | |
339–344 | Ditto. While this one is pretty short, I'm don't imagine there's any benefit to having it be inline. | |
444 | Rename ts to tensors and ds to dims. | |
447–450 | Can any of these be marked const? | |
454–466 | Move this documentation to just above the struct LoopSeqInfo { rather than having it inside the class definition | |
469 | Constructors should be moved to the top of the class / fields should be at the bottom of the class | |
473–475 | If this comment is for loopStack then it should be placed above the loopStack declaration. Whereas, if this comment is for loopSeqStack then it should have a blank line separating the comment from the loopStack declaration and shouldn't have any blank lines between the comment and the loopSeqStack declaration. |
mark varaible as const.
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.cpp | ||
---|---|---|
173–179 | I think isCompressedDim looks good to me, I will leave the decision to Aart. I will change the dims though! Thanks! | |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
297 | Sure, we can have some discussion on this! | |
324–335 | Will do | |
447–450 | You are right! | |
469 | Deleted... |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
---|---|---|
444 | I changed ts -> tids ( because there is a tensors field in the loop emitter that causes conflicts) hope it looks good to you ;-) |
mlir/lib/Dialect/SparseTensor/Transforms/CodegenUtils.h | ||
---|---|---|
251 | something went wrong with the rebase? this does not seem related? | |
269 | you moved the class and changed it. perhaps this is a better place, but that makes the diff either harder to read. can we first modify, and then later (once it is in), move the class, that makes it easier to read | |
mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
196 | This seems a functional change not really required for this revision, right? | |
408 | a lot of these changes are because we removed vectorization as discussed, that is okay, but this is something that can be done in a separate revision, right before this one, just so that the amount of changes per revision goes down | |
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_cast.mlir | ||
7 | Here and below, for all integration tests (not the CHECK tests) that are run without and with vectorization, simply remove the L7-L13 part. I will be easy to add that back later, so nothing is lost by removing it (in fact, at one point, we may want to run *all* integration tests with and without vectorization by some other means) |
something went wrong with the rebase? this does not seem related?