[PATCH] D78498: [mlir] [linalg] Only promote selected buffers.

Tue Apr 21 03:12:38 PDT 2020

ftynse accepted this revision.
ftynse added inline comments.
This revision is now accepted and ready to land.

================
Comment at: mlir/include/mlir/Dialect/Linalg/Transforms/LinalgTransforms.h:127
+/// `operandIndicesToPromote`.
+/// If linalgMarker is specified and the transformation is successfull
+/// sets the attribute `kLinalgTransformMarker` to `linalgMarker`.
----------------
Nit: typo "successful". And grammo: put a comma after an if-clause.

================
Comment at: mlir/test/lib/DeclarativeTransforms/TestLinalgTransformPatterns.td:153
+def : Pat<(MatmulOp:$op $_, $_, $_),
+          (PromoteSelectedSubviewsLinalgOp<[0, 1], "first_view_promotion">),
+          [(Constraint<And<[
----------------
poechsel wrote:
> ftynse wrote:
> > I don't follow this. The test name suggests you promote the _first_ view, but the list contains operands with indices 0 and 1... The .mlir file also seems to check only for one copy. Consider using only `[0]` here and `CHECK-NOT` for extra copies before matmul.
> Nice catch! 
> 
> I believe there is a problem with `test/Dialect/Linalg/transform-patterns.mlir` and that the test is passing even when it should fail:
> - in my case I should have had a fallback on line 439 where I'm checking the ssa address of arg1 with the one it should have if no promotion happens.
> - since D77676 promotion shouldn't generate `linalg.slice` operations but this test is still checking for their presence (ex: line 393).
Indeed, after further investigation, the test above uses wrong syntax for FileCheck. There must be no spaces between `CHECK` and `:` @nicolasvasilache.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78498