[VectorOps] Implement a simple folder for identity vector.transpose operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Is there a followup revision planned in which you would canonicalize transpose(transpose()) and have it compose with these newly added folders?
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
---|---|---|
1540 | Please run clang-format. |
please label title with mlir as well
[mlir] [VectorOps]
mlir/include/mlir/Dialect/Vector/VectorOps.td | ||
---|---|---|
1318 | note that the attribute is really called "transp" (perhaps not such a good name looking back), so getTransp or getTranspArray would be slightly more in style; but I am okay with this too and perhaps rename the array later | |
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
1535 | wouldn't it read a little easier as for (int64_t i = 0, n = permutation.size; i < n; i++) if (permutation[i] != i) ... i.e. less casting? | |
mlir/test/Dialect/Vector/canonicalize.mlir | ||
132 | maybe for completeness of what you have right now also a combination of transpose-ID and transpose-non-ID to make sure only the latter remain? |
Thanks, Nicolas, yes, I expect to make an update to eliminate combinations of transposes.
Good to go, but some last comments to consider in the follow up.
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
---|---|---|
1536 | Given the llvm style of leaving the {} out of trivial for/ifs (something I am not a big fan of), I am wondering if that actually also applies to this case, trivial for loop with trivial if, so no {} on for either; perhaps others will comment, otherwise just leave it | |
mlir/test/Dialect/Vector/canonicalize.mlir | ||
155 | this of course captures that the intermediate tranpose is gone, but to be completely consistent with your other tests, you would expect a CHECK-NOT here (to make sure it is also removed as dead code) |
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
---|---|---|
1536 | I can turn this loop (back) into a lambda. |
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
---|---|---|
1536 | Submitting as is. |
note that the attribute is really called "transp" (perhaps not such a good name looking back), so getTransp or getTranspArray would be slightly more in style; but I am okay with this too and perhaps rename the array later