[PATCH] D79135: [mlir][Linalg] Add support to lower named ops to loops.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:126
+/// Emits the MLIR for the scalar part of the generic op by:
+///   1. Emitting std_load and std_store ops for each input and output
+///      view in order. This is achieved by applying the appropriate input or
----------------
Does "1." actually emit stores?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:133
+///      views.
+//
+/// An example output may resemble:
----------------
Nit: `//` -> `///`


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:3
 // RUN: mlir-opt %s -convert-linalg-to-parallel-loops | FileCheck --check-prefix=CHECKPARALLEL %s
+// RUN: mlir-opt %s -convert-linalg-to-affine-loops --disable-pass-threading | FileCheck --check-prefix=CHECKAFFINE %s
 
----------------
bondhugula wrote:
> Nit: I think you have a separate file affine.mlir for -convert-linalg-to-affine-loops?
Why do you need to disable pass threading here? 


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:6
 // Test that we can lower all the way to LLVM without crashing, don't check results here.
-// RUN: mlir-opt %s --convert-linalg-to-llvm -o=/dev/null 2>&1
+// RUN: mlir-opt %s -convert-linalg-to-loops --convert-linalg-to-llvm -o=/dev/null 2>&1
 
----------------
I'm surprised this ever worked on non-unix system


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:892
+//       CHECKLOOP:       loop.for %[[k:.*]] = %{{.*}} to %[[K]] step %{{.*}} {
+//   CHECKLOOP-DAG:       %[[va:.*]] = load %[[mA]][%[[b]], %[[m]], %[[k]]] : memref<?x?x?xf32>
+//   CHECKLOOP-DAG:       %[[vb:.*]] = load %[[mB]][%[[b]], %[[k]], %[[n]]] : memref<?x?x?xf32>
----------------
The comment in the code says loads are emitted in the same order as operands, so why "DAG" ?


================
Comment at: mlir/test/Dialect/Linalg/loops.mlir:895
+//   CHECKLOOP-DAG:       %[[vc:.*]] = load %[[mC]][%[[b]], %[[m]], %[[n]]] : memref<?x?x?xf32>
+//   CHECKLOOP-DAG:       %[[inc:.*]] = mulf %[[va]], %[[vb]] : f32
+//   CHECKLOOP-DAG:       %[[res:.*]] = addf %[[vc]], %[[inc]] : f32
----------------
DAG here looks like it would allow breaking use-def chains...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79135