[PATCH] D77088: [mlir] [VectorOps] Implement a simple folder for identity vector.transpose operations.

[Aart Bik via Phabricator via llvm-commits]

aartbik accepted this revision.
aartbik added a comment.

Good to go, but some last comments to consider in the follow up.



================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1536
+  // {0, 1, 2, ...}.
+  for (int64_t i = 0, e = transp.size(); i < e; i++) {
+    if (transp[i] != i)
----------------
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


================
Comment at: mlir/test/Dialect/Vector/canonicalize.mlir:155
+  %4 = vector.transpose %3, [0, 1, 2] : vector<2x3x4xf32> to vector<2x3x4xf32>
+  // CHECK-NEXT: return [[ADD]]
+  return %4 : vector<2x3x4xf32>
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77088/new/

https://reviews.llvm.org/D77088