[PATCH] D74954: Add a basic tiling pass for parallel loops

[Stephan Herhut via Phabricator via llvm-commits]

herhut requested changes to this revision.
herhut added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:24
+
+static llvm::cl::opt<unsigned>
+    clTileSize("parallel-loop-tile-size",
----------------
Please use pass options.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:29
+
+// Tile a ploop of the form
+//   loop.parallel (%i0, %i1) = (%arg0, %arg1) to (%arg2, %arg3)
----------------
nit: Doc comments need to start with `///`

Use parallel loop instead of ploop.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:41
+// The old loops is replaced with the new one.
+static void tilePLoop(ParallelOp op, int64_t tileSize) {
+  OpBuilder b(op);
----------------
tileParallelLoop


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:43
+  OpBuilder b(op);
+  SmallVector<Value, 2> newSteps;
+  auto zero = b.create<ConstantIndexOp>(op.getLoc(), 0);
----------------
Maybe `newSteps.reserve`?


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:67
+  for (auto bounds : llvm::zip(op.upperBound(), op.lowerBound())) {
+    newBounds.push_back(b.create<AffineMinOp>(
+        op.getLoc(), b.getIndexType(), minMap,
----------------
I do not understand this part. We want the min of either the step size or dim - upperbound_of_outer.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:76
+  // Steal the body of the old ploop.
+  innerLoop.getBody()->getOperations().splice(
+      innerLoop.getBody()->getOperations().begin(),
----------------
you can do something like innerLoop.region().takeBody(op.region()) which simply transfers one region to the other. As the number of block arguments does not change, no need to fix uses or anything.


================
Comment at: mlir/lib/Dialect/LoopOps/Transforms/ParallelLoopTiling.cpp:94
+                                    SmallVectorImpl<ParallelOp> &loops) {
+  for (auto &op : *block) {
+    if (auto parallelOp = dyn_cast<ParallelOp>(op)) {
----------------
`getBlock().getOps<loop::ParallelOp>()` gives an (potentially empty) iterator over all parallel loops in the block.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74954