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

[Alexander Belyaev via Phabricator via llvm-commits]

pifon2a added a comment.

@rriddle @ftynse Thank you for the comments!



================
Comment at: mlir/include/mlir/Dialect/Linalg/Passes.h:38
+/// std.load/std.store accesses.
+std::unique_ptr<OpPassBase<FuncOp>> createConvertLinalgToParallelLoopsPass();
+
----------------
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.


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.h:47
+/// Returns the parallel loop parent of an induction variable. If the provided
+// value is not an induction variable, then return nullptr.
+ParallelOp getParallelForInductionVarOwner(Value val);
----------------
rriddle wrote:
> ///
There are also comments for these functions in Passes.h. I am removing the comments from .cpp.


================
Comment at: mlir/lib/EDSC/Builders.cpp:196
+  auto opHandle = OperationHandle::create<loop::ParallelOp>(
+      SmallVector<Value, 4>(lbHandles.begin(), lbHandles.end()),
+      SmallVector<Value, 4>(ubHandles.begin(), ubHandles.end()),
----------------
rriddle wrote:
> llvm::to_vector<4>(lbHandles|ubHandles|steps)
this did not work


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73684