[PATCH] D74544: [MLIR] Add naive fusion of parallel loops.

[River Riddle via Phabricator via llvm-commits]

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


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:21
+
+namespace mlir {
+namespace {
----------------
using namespace mlir;



================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:26
+
+// Verify there are no nested ParallelOps.
+bool hasNestedParallelOp(ParallelOp ploop) {
----------------
Use /// for top-level comments.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:27
+// Verify there are no nested ParallelOps.
+bool hasNestedParallelOp(ParallelOp ploop) {
+  auto walkResult = ploop.getBody()->walk(
----------------
Only classes should be in anonymous namespaces, functions should be in the global namespace and marked static.

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:29
+  auto walkResult = ploop.getBody()->walk(
+      [](ParallelOp) -> WalkResult { return WalkResult::interrupt(); });
+  return walkResult.wasInterrupted();
----------------
nit: -> WalkResult is unnecessary.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:40
+                           const OperandRange &rhs) -> bool {
+    for (auto item : llvm::zip(lhs, rhs)) {
+      Value p1, p2;
----------------
nit: Can you just use std::equal for now?


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:56
+// writes to the same buffer elements that ploop2 reads.
+bool verifyDependencies(ParallelOp ploop1, ParallelOp ploop2,
+                        const BlockAndValueMapping &map) {
----------------
nit: Use LogicalResult instead of boolean.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:75
+    auto loadIndices = load.indices();
+    if (storeIndices.size() != loadIndices.size()) {
+      return WalkResult::interrupt();
----------------
Drop trivial braces.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:85
+  });
+  bool result = !walkResult.wasInterrupted();
+  return result;
----------------
nit: Remove this temporary value and just return the result directly. It doesn't really help readability at all.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:117
+    // Not using `walk()` to traverse only top-level parallel loops.
+    for (auto &op : block.without_terminator()) {
+      if (auto ploop = dyn_cast<ParallelOp>(op)) {
----------------
nit: `block.getOps<ParallelOp>()`


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:123
+    // Iteratively fuse adjacent loops.
+    for (int i = 0, e = ploops.size(); i + 1 < e; ++i) {
+      fuseIfLegal(ploops[i], ploops[i + 1], b);
----------------
nit: Drop all of these trivial braces.


================
Comment at: mlir/lib/Transforms/ParallelLoopFusion.cpp:136
+
+static mlir::PassRegistration<mlir::LoopFusion>
+    pass("parallel-loop-fusion", "Fuse parallel loop nests");
----------------
Drop the mlir:: on each of these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74544