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

[Alex Grosul via Phabricator via llvm-commits]

grosul1 marked 3 inline comments as done.
grosul1 added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Vector/VectorOps.td:1318
     }
+    void getPermutation(SmallVectorImpl<int64_t> &results);
   }];
----------------
aartbik wrote:
> 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
Done! Renamed everything to transp/Transp, no discrepancy for now. Everything can be renamed at once if/when someone gets to do it.


================
Comment at: mlir/lib/Dialect/Vector/VectorOps.cpp:1535
+  // Check if the permutation contains sequential values: {0, 1, 2, ...}.
+  for (size_t i = 0; i < permutation.size(); i++) {
+    if (permutation[i] != static_cast<int64_t>(i))
----------------
aartbik wrote:
> 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?
Used "e = " as it seems to be a convention; in this file, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77088