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

[Stephan Herhut via Phabricator via llvm-commits]

herhut accepted this revision.
herhut marked an inline comment as done.
herhut added a comment.

Whoohoo. \0/ Thanks!



================
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:
> > 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.
> We will have tiling and fusion on Ploops before going to GPU.
I think you are cross-talking here. What ftynse suggests is always going

LinAlg->Ploops->SeqLoops->... 

instead of also having

LinAlg->SeqLoops.

That is a good point. I would suggest we build out LinAlg->Ploop first before retiring the other lowering, though. 


================
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);
+
----------------
ftynse wrote:
> 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.
Just adding my 2 //<insert small change of currency of choice>// here. I find the name body misleading, especially when there also is a getBody() method on the same op. Hence I prefer region for the region and body to return the first block (if there is a single block in the region).


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:234
+  Region *bodyRegion = result.addRegion();
+  ForOp::ensureTerminator(*bodyRegion, *builder, result.location);
+  for (size_t i = 0; i < steps.size(); ++i)
----------------
ParallelOp::ensureTerminator


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:329
   // Add a terminator if none was parsed.
   ForOp::ensureTerminator(*body, builder, result.location);
 
----------------
I just saw it is wrong here, too :(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73684