This patch moves some utils into CodegenEnv class, it should make the code easier to follow and it eliminates several indirect value assignment that use ptr**.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| mlir/lib/Dialect/SparseTensor/Transforms/CodegenEnv.cpp | ||
|---|---|---|
| 91 | These section names correspond 1:1 with the comments in the header file. So I would org them consistently | |
| mlir/lib/Dialect/SparseTensor/Transforms/CodegenEnv.h | ||
| 45 | empty line after the general methods section | |
| 54 | I would group initTensorExp/getTensorExp together | |
| mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
| 1444 | is this a functional change? | |
| mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
|---|---|---|
| 1444 | Maybe? but not really? it was in isAdmissibleTensorExp that was called in the loop that iterates over sort masks, but this value is loop invariant, so i think it might be better to move it out to reject the kernel earlier. (See L568 in the original code) | |
| mlir/lib/Dialect/SparseTensor/Transforms/CodegenEnv.cpp | ||
|---|---|---|
| 91 | The order does not match the header. In the header we have this order topo / verify / output but in the CC you have verify / topo / output Probably easiest to just fix this in header? | |
| 131 | here and below, no longer needed to call op() | |
| 136 | here and below, no longer need to call merger() | |
| mlir/lib/Dialect/SparseTensor/Transforms/CodegenEnv.h | ||
| 110 | /// style more consistent with rest | |
| 112 | empty line before second comment/method | |
| 112 | You need to switch the order here in the header admissibleTensorExp appears before admissibleTopoOrder in CC file (also, that is the logical order of calling, so I like to see that reflected here) | |
| 112 | also, add // Sets outerParNest on success with sparse output | |
| 113 | Setting sparseOutput is also rather essential in making sure we can split it into two admissible methods, i.e. first admissable tensor exp then admissible topoorder. I had to think about that for a bit! | |
| mlir/lib/Dialect/SparseTensor/Transforms/Sparsification.cpp | ||
| 1444 | Fair enough but, AFAIK, the "NFC" tag is really reserved for textual refactoring only, not for something that actually changes behavior (even if the overall behavior stays the same) ;-) | |
| 1458–1460 | Keep a comment here on what it does | |
empty line after the general methods section