[PATCH] D74880: [mlir] [VectorOps] Multi-dim reductions for lowering vector.contract

[Aart Bik via Phabricator via llvm-commits]

aartbik marked 4 inline comments as done.
aartbik added a comment.

The order is always the same of a given mapping and op, but the accumulation may indeed introduce instabilities if an exact order is assumed (i.e. acc feeds into the chain rather than being just added at the end). We can make semantics more strict if needed.



================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:1031
+        rewriter.getArrayAttr(adjustIter(op.iterator_types(), iterIndex));
+    // Unroll into a series of lower dimensional vector.contract ops.
+    // By feeding the initial accumulator into the first contraction,
----------------
andydavis1 wrote:
> Maybe not in this change, but it would be great if this unrolling code could reuse the unrolling code already in this file: mlir::vector::unrollSingleResultOpMatchingType(
Ack.


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:1070
+      int64_t idx = it.index();
+      if (idx == index) {
+        continue;
----------------
andydavis1 wrote:
> s/idx/id.index()  don't see 'idx' used elsewhere.
This was actually done on purpose to avoid having to cast one to the type of the other (to avoid compiler complaints about sign).
By first assigning it.index() to int64_t this is all done in a much more readable way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74880