[PATCH] D73684: [MLIR][Linalg] Lower linalg.generic to ploops.

[Alex Zinenko via Phabricator via llvm-commits]

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


================
Comment at: mlir/include/mlir/Dialect/Linalg/Passes.h:38
+/// std.load/std.store accesses.
+std::unique_ptr<OpPassBase<FuncOp>> createConvertLinalgToParallelLoopsPass();
+
----------------
pifon2a wrote:
> ftynse wrote:
> > Given that we have parallel loops to sequential loops, does it make sense to keep Linalg to sequential loops?
> Yes, it does. We want to go LHLO->Linalg->Ploops->GPU.
Why? How having Linalg->Loops helps you? You are going to have LHLO->Linalg->Ploops->GPU, and you can also have LHLO->Linalg->Ploops->Loops->e.g.CPU because we have Ploops->Loops. I don't see the need for Linalg->Loops in this scheme, it will be mostly duplicate code.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:179
   let results = (outs Variadic<AnyType>:$results);
-  let regions = (region SizedRegion<1>:$body);
+  let regions = (region SizedRegion<1>:$region);
+
----------------
pifon2a wrote:
> ftynse wrote:
> > Any specific reason for this rename?  I find "body" to be a better name than 'region".
> This is what we have in loop::ForOp. Just wanted to be consistent. I can change both though. I can also change getBody()->body() in ForOp and in ParallelOp.
Consistency is good. Change it in a follow-up if you want.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:416
+template <typename LoopTy>
+bool LoweringIsAllowed(int numParallelLoops, int numLoops) {
+  return true;
----------------
Nit: MLIR uses lowerCamelCase for function names


================
Comment at: mlir/test/Dialect/Linalg/parallel_loops.mlir:1
+// RUN: mlir-opt %s -convert-linalg-to-parallel-loops -split-input-file | FileCheck %s --dump-input=always
+
----------------
Do we need really need `--dump-input=always` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73684