[PATCH] D76014: [mlir][Vector]Lower vector.contract to llvm.intr.matrix_multiply

[Aart Bik via Phabricator via llvm-commits]

aartbik added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Utils/StructuredOpsUtils.h:42
+  auto mapB = AffineMapAttr::get(AffineMap::get(3, 0, {m, k}));
+  auto mapC = AffineMapAttr::get(AffineMap::get(3, 0, {n, m}));
+  auto maps = ArrayAttr::get({mapA, mapB, mapC}, indexingMaps.getContext());
----------------
note that we also have two "transposed" versions, ie. with either {m,n} or {n.m} but then the position of the k's swapped


================
Comment at: mlir/lib/Dialect/VectorOps/VectorTransforms.cpp:921
+      Type elementType = op.getLhsType().getElementType();
+      if (elementType.isIntOrFloat()) {
+        if (elementType.isSignedInteger() || elementType.isSignedInteger())
----------------
can this test every fail? if so, shouldn't we move it a bit up to avoid doing some work first?


================
Comment at: mlir/test/Dialect/VectorOps/vector-contract-transforms.mlir:1
 // RUN: mlir-opt %s -test-vector-contraction-conversion | FileCheck %s
+// RUN: mlir-opt %s -test-vector-contraction-conversion -vector-lower-matrix-intrinsics | FileCheck %s --check-prefix=MATRIX
----------------
note that in the pending CL, I have renamed this  flag and file, since it started to become less and less about contract only :-)

One of us will have to rebase and merge


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76014