[PATCH] D76889: [MLIR][Vector] Add support for TupleGetOp folding through InsertSlicesOp and ExtractSlicesOp.

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:677
 
+// Returns the producer Value of the same type as 'consumerValue', by tracking
+// the tuple index and offsets of the consumer vector value through the
----------------
nit: // -> ///


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:692
+    if (auto tupleGetOp = dyn_cast<vector::TupleGetOp>(op)) {
+      assert(tupleIndex == -1);
+
----------------
Please add a comment to this assert.


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:715
+      assert(offsets.size() == elementOffsets.size());
+      for (unsigned i = 0, e = offsets.size(); i < e; ++i) {
+        offsets[i] += elementOffsets[i];
----------------
nit: Drop all trivial braces.


================
Comment at: mlir/lib/Dialect/Vector/VectorTransforms.cpp:758
+      auto value = tupleOp.getOperand(tupleIndex);
+      if (auto producerVectorType = value.getType().cast<VectorType>()) {
+        if (producerVectorType == consumerVectorType)
----------------
cast never returns null, did you intend to use dyn_cast here? Also, can you just merge these two predicates:

```
if (value.getType() == consumerVectorType)
   ...
```
?


================
Comment at: mlir/lib/Dialect/Vector/VectorUtils.cpp:31
 
 namespace mlir {
 
----------------
We should not be doing this inside of .cpp files. It should be `using namespace mlir` with the functions explicitly qualified.

https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76889