[PATCH] D73796: [mlir][Linalg] Adding support for linalg_matmul with tensors.

[Nicolas Vasilache via Phabricator via llvm-commits]

nicolasvasilache marked 4 inline comments as done.
nicolasvasilache added inline comments.


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:1032
+  auto f32Type = FloatType::getF32(&globalContext());
+  auto memrefType = MemRefType::get({-1, -1}, f32Type, {}, 0);
+  auto tensorType = RankedTensorType::get({-1, -1}, f32Type);
----------------
antiagainst wrote:
> Do we want to use `ShapedType::kDynamicSize` for these magic numbers?
good point, I'll update everything in the test file in a followup CL.


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:1042
+  StructuredIndexed SA(A), SB(B), SC(tensorType);
+  linalg_pointwise_add(SA({i, j}), SB({i, j}), SC({i, j}));
+  linalg_pointwise_max(SA({i, j}), SB({i, j}), SC({i, j}));
----------------
antiagainst wrote:
> I'm not sure about the conventions here but I'd typically prefer to have small and focused tests so it's easier to maintain and upate.
Right, so the tradeoff is bigger test vs boilerplate copy-pasta.
I'd prefer bigger test, up to some (undetermined) reasonable limit, at this time the test goes from 3 to 5 ops.
This falls into my reasonable category but I'll split if you have a strong preference for the copy-pasta.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73796