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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h:80
+  using LoopOrAffineLoopBuilder =
+      typename std::conditional<std::is_same<LoopTy, AffineForOp>::value,
+                                AffineLoopNestBuilder,
----------------
nit: conditional_t here and below


================
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);
----------------
///


================
Comment at: mlir/include/mlir/Dialect/LoopOps/LoopOps.td:184
+    OpBuilder<"Builder *builder, OperationState &result, "
+              "ValueRange lowerBounds, ValueRange upperBounds, ValueRange steps">
+  ];
----------------
Is this wrapped at 80?


================
Comment at: mlir/include/mlir/EDSC/Builders.h:224
 
+class ParallelLoopNestBuilder {
+public:
----------------
Can you add a comment to this?


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:119
+    ArrayRef<ValueHandle *> ivs, ArrayRef<Value> ranges) {
+  SmallVector<ValueHandle, 4> lbs;
+  SmallVector<ValueHandle, 4> ubs;
----------------
nit: lbs, ubs, steps?


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:613
 
+// Emits a loop nest of `loop.parallel` with the proper body for `op`.
+template <typename ConcreteOp>
----------------
///


================
Comment at: mlir/lib/Dialect/LoopOps/LoopOps.cpp:351
+  auto *containingInst = ivArg.getOwner()->getParentOp();
+  return dyn_cast_or_null<ParallelOp>(containingInst);
+}
----------------
dyn_cast?


================
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()),
----------------
llvm::to_vector<4>(lbHandles|ubHandles|steps)


================
Comment at: mlir/lib/EDSC/Builders.cpp:202
+      cast<loop::ParallelOp>(*opHandle.getOperation());
+  for (size_t i = 0; i < ivs.size(); ++i) {
+    *ivs[i] = ValueHandle(parallelOp.getBody()->getArgument(i));
----------------
*Drop trivial braces
*Cache the end iterator of for loops.


================
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
+
----------------
ftynse wrote:
> Do we need really need `--dump-input=always` ?
`dump-input-on-failure` is a better choice here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73684