[mlir][omp] Improve canonloop/iv naming #159773

Meinersbur · 2025-09-19T13:00:29Z

Improve the automatic naming of variables defined by the omp.canonical_loop operation:

The iteration variable gets a name consistent with the cli variable
Instead of appending _s0 for each nesting level, shorten it to _d<num> for a perfectly nested loop at depth <num>.
Do not add any suffix to the top-level loop if it is the only top-level loop

Part of a PR Stack:

llvmbot · 2025-09-24T10:31:05Z

@llvm/pr-subscribers-mlir-openmp
@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-mlir

Author: Michael Kruse (Meinersbur)

Changes

Improve the automatic naming of variables defined by the omp.canonical_loop operation:

The iteration variable get a name consistent with the cli variable
Instead of appending _s0 for each nesting level, shorten it to _d<num> for a perfectly nested loop at depth <num>.
Do not add any suffix to the top-level loop if it is the only top-level loop

Full diff: https://github.com/llvm/llvm-project/pull/159773.diff

3 Files Affected:

(modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+174-62)
(modified) mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir (+103-24)
(modified) mlir/test/Dialect/OpenMP/cli-unroll-heuristic.mlir (+14-14)

diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index 3d70e28ed23ab..1674891410194 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -77,6 +77,177 @@ struct LLVMPointerPointerLikeModel
 };
 } // namespace
 
+/// Generate a name of a canonical loop nest of the format
+/// `<prefix>(_s<num>_r<num>)*` that describes its nesting inside parent
+/// operations (`_r<num>`) and that operation's region (`_s<num>`). The region
+/// number is omitted if the parent operation has just one region. If a loop
+/// nest just consists of canonical loops nested inside each other, also uses
+/// `d<num>` where <num> is the nesting depth of the loop.
+static std::string generateLoopNestingName(StringRef prefix,
+                                           CanonicalLoopOp op) {
+  struct Component {
+    // An region argument of an operation
+    Operation *parentOp;
+    size_t regionInOpIdx;
+    bool isOnlyRegionInOp;
+    bool skipRegion;
+
+    // An operation somewhere in a parent region
+    Operation *thisOp;
+    Region *parentRegion;
+    size_t opInRegionIdx;
+    bool isOnlyOpInRegion;
+    bool skipOp;
+    int depth = -1;
+  };
+  SmallVector<Component> components;
+
+  // Gather a list of parent regions and operations, and the position within
+  // their parent
+  Operation *o = op.getOperation();
+  while (o) {
+    if (o->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>())
+      break;
+
+    // Operation within a region
+    Region *r = o->getParentRegion();
+    if (!r)
+      break;
+
+    llvm::ReversePostOrderTraversal<Block *> traversal(&r->getBlocks().front());
+    size_t idx = 0;
+    bool found = false;
+    size_t sequentialIdx = -1;
+    bool isOnlyLoop = true;
+    for (Block *b : traversal) {
+      for (Operation &op : *b) {
+        if (&op == o && !found) {
+          sequentialIdx = idx;
+          found = true;
+        }
+        if (op.getNumRegions()) {
+          idx += 1;
+          if (idx > 1)
+            isOnlyLoop = false;
+        }
+        if (found && !isOnlyLoop)
+          break;
+      }
+    }
+
+    Component &comp = components.emplace_back();
+    comp.thisOp = o;
+    comp.parentRegion = r;
+    comp.opInRegionIdx = sequentialIdx;
+    comp.isOnlyOpInRegion = isOnlyLoop;
+
+    // Region argument of an operation
+    Operation *parent = r->getParentOp();
+
+    comp.parentOp = parent;
+    comp.regionInOpIdx = 0;
+    comp.isOnlyRegionInOp = true;
+    if (parent && parent->getRegions().size() > 1) {
+      auto getRegionIndex = [](Operation *o, Region *r) {
+        for (auto [idx, region] : llvm::enumerate(o->getRegions())) {
+          if (&region == r)
+            return idx;
+        }
+        llvm_unreachable("Region not child of its parent operation");
+      };
+      comp.regionInOpIdx = getRegionIndex(parent, r);
+      comp.isOnlyRegionInOp = false;
+    }
+
+    if (!parent)
+      break;
+
+    // next parent
+    o = parent;
+  }
+
+  // Reorder components from outermost to innermost
+  std::reverse(components.begin(), components.end());
+
+  // Determine whether a component is not needed
+  for (Component &c : components) {
+    c.skipRegion = c.isOnlyRegionInOp;
+    c.skipOp = c.isOnlyOpInRegion && !isa<CanonicalLoopOp>(c.thisOp);
+  }
+
+  // Find runs of perfect nests and merge them into a single component
+  size_t curNestRoot = 0;
+  size_t curNestDepth = 1;
+  auto mergeLoopNest = [&](size_t innermost) {
+    size_t outermost = curNestRoot;
+
+    // Don't do enything if it does not consist of at least 2 loops
+    if (outermost < innermost) {
+      for (auto i : llvm::seq<int>(outermost + 1, innermost))
+        components[i].skipOp = true;
+      components[innermost].depth = curNestDepth;
+    }
+
+    // Start new root
+    curNestRoot = innermost + 1;
+    curNestDepth = 1;
+  };
+  for (auto &&[i, c] : llvm::enumerate(components)) {
+    if (i <= curNestRoot)
+      continue;
+
+    // Check whether this region can be included
+    if (!c.skipRegion) {
+      mergeLoopNest(i);
+      continue;
+    }
+
+    if (c.skipOp)
+      continue;
+
+    if (!c.isOnlyOpInRegion) {
+      mergeLoopNest(i);
+      continue;
+    }
+
+    curNestDepth += 1;
+  }
+
+  // Finalize innermost loop nest
+  mergeLoopNest(components.size() - 1);
+
+  // Outermost loop does not need a suffix if it has no sibling
+  for (auto &c : components) {
+    if (c.skipOp)
+      continue;
+    if (c.isOnlyOpInRegion)
+      c.skipOp = true;
+    break;
+  }
+
+  // Compile name
+  SmallString<64> Name{prefix};
+  for (auto &c : components) {
+    auto addComponent = [&Name](char letter, int64_t idx) {
+      Name += '_';
+      Name += letter;
+      Name += idx;
+    };
+
+    if (!c.skipRegion)
+      addComponent('r', c.regionInOpIdx);
+
+    if (!c.skipOp) {
+      if (c.depth >= 0)
+        addComponent('d', c.depth - 1);
+      else
+        addComponent('s', c.opInRegionIdx);
+    }
+  }
+
+  return Name.str().str();
+}
+
 void OpenMPDialect::initialize() {
   addOperations<
 #define GET_OP_LIST
@@ -3141,67 +3312,7 @@ void NewCliOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) {
     cliName =
         TypeSwitch<Operation *, std::string>(gen->getOwner())
             .Case([&](CanonicalLoopOp op) {
-              // Find the canonical loop nesting: For each ancestor add a
-              // "+_r<idx>" suffix (in reverse order)
-              SmallVector<std::string> components;
-              Operation *o = op.getOperation();
-              while (o) {
-                if (o->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>())
-                  break;
-
-                Region *r = o->getParentRegion();
-                if (!r)
-                  break;
-
-                auto getSequentialIndex = [](Region *r, Operation *o) {
-                  llvm::ReversePostOrderTraversal<Block *> traversal(
-                      &r->getBlocks().front());
-                  size_t idx = 0;
-                  for (Block *b : traversal) {
-                    for (Operation &op : *b) {
-                      if (&op == o)
-                        return idx;
-                      // Only consider operations that are containers as
-                      // possible children
-                      if (!op.getRegions().empty())
-                        idx += 1;
-                    }
-                  }
-                  llvm_unreachable("Operation not part of the region");
-                };
-                size_t sequentialIdx = getSequentialIndex(r, o);
-                components.push_back(("s" + Twine(sequentialIdx)).str());
-
-                Operation *parent = r->getParentOp();
-                if (!parent)
-                  break;
-
-                // If the operation has more than one region, also count in
-                // which of the regions
-                if (parent->getRegions().size() > 1) {
-                  auto getRegionIndex = [](Operation *o, Region *r) {
-                    for (auto [idx, region] :
-                         llvm::enumerate(o->getRegions())) {
-                      if (&region == r)
-                        return idx;
-                    }
-                    llvm_unreachable("Region not child its parent operation");
-                  };
-                  size_t regionIdx = getRegionIndex(parent, r);
-                  components.push_back(("r" + Twine(regionIdx)).str());
-                }
-
-                // next parent
-                o = parent;
-              }
-
-              SmallString<64> Name("canonloop");
-              for (const std::string &s : reverse(components)) {
-                Name += '_';
-                Name += s;
-              }
-
-              return Name;
+              return generateLoopNestingName("canonloop", op);
             })
             .Case([&](UnrollHeuristicOp op) -> std::string {
               llvm_unreachable("heuristic unrolling does not generate a loop");
@@ -3292,7 +3403,8 @@ void CanonicalLoopOp::getAsmBlockNames(OpAsmSetBlockNameFn setNameFn) {
 
 void CanonicalLoopOp::getAsmBlockArgumentNames(Region &region,
                                                OpAsmSetValueNameFn setNameFn) {
-  setNameFn(region.getArgument(0), "iv");
+  std::string ivName = generateLoopNestingName("iv", *this);
+  setNameFn(region.getArgument(0), ivName);
 }
 
 void CanonicalLoopOp::print(OpAsmPrinter &p) {
diff --git a/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir b/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir
index adadb8bbac49d..874e3922805ec 100644
--- a/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir
+++ b/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir
@@ -1,5 +1,5 @@
-// RUN: mlir-opt %s | FileCheck %s
-// RUN: mlir-opt %s | mlir-opt | FileCheck %s
+// RUN: mlir-opt %s | FileCheck %s --enable-var-scope
+// RUN: mlir-opt %s | mlir-opt | FileCheck %s --enable-var-scope
 
 
 // CHECK-LABEL: @omp_canonloop_raw(
@@ -24,10 +24,10 @@ func.func @omp_canonloop_raw(%tc : i32) -> () {
 func.func @omp_canonloop_sequential_raw(%tc : i32) -> () {
   // CHECK-NEXT: %canonloop_s0 = omp.new_cli
   %canonloop_s0 = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
+  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s0) %iv_s0 : i32 in range(%[[tc]]) {
   "omp.canonical_loop" (%tc, %canonloop_s0) ({
     ^bb_first(%iv_first: i32):
-      // CHECK-NEXT: = llvm.add %iv, %iv : i32
+      // CHECK-NEXT: = llvm.add %iv_s0, %iv_s0 : i32
       %newval = llvm.add %iv_first, %iv_first : i32
     // CHECK-NEXT: omp.terminator
     omp.terminator
@@ -36,7 +36,7 @@ func.func @omp_canonloop_sequential_raw(%tc : i32) -> () {
 
   // CHECK-NEXT: %canonloop_s1 = omp.new_cli
   %canonloop_s1 = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT: omp.canonical_loop(%canonloop_s1) %iv : i32 in range(%[[tc]]) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop_s1) %iv_s1 : i32 in range(%[[tc]]) {
   "omp.canonical_loop" (%tc, %canonloop_s1) ({
     ^bb_second(%iv_second: i32):
     // CHECK: omp.terminator
@@ -52,17 +52,17 @@ func.func @omp_canonloop_sequential_raw(%tc : i32) -> () {
 // CHECK-LABEL: @omp_nested_canonloop_raw(
 // CHECK-SAME: %[[tc_outer:.+]]: i32, %[[tc_inner:.+]]: i32)
 func.func @omp_nested_canonloop_raw(%tc_outer : i32, %tc_inner : i32) -> () {
-  // CHECK-NEXT: %canonloop_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop = omp.new_cli
   %outer = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT: %canonloop_s0_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop_d1 = omp.new_cli
   %inner = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT: omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc_outer]]) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc_outer]]) {
   "omp.canonical_loop" (%tc_outer, %outer) ({
     ^bb_outer(%iv_outer: i32):
-      // CHECK-NEXT: omp.canonical_loop(%canonloop_s0_s0) %iv_0 : i32 in range(%[[tc_inner]]) {
+      // CHECK-NEXT: omp.canonical_loop(%canonloop_d1) %iv_d1 : i32 in range(%[[tc_inner]]) {
       "omp.canonical_loop" (%tc_inner, %inner) ({
         ^bb_inner(%iv_inner: i32):
-          // CHECK-NEXT: = llvm.add %iv, %iv_0 : i32
+          // CHECK-NEXT: = llvm.add %iv, %iv_d1 : i32
           %newval = llvm.add %iv_outer, %iv_inner: i32
           // CHECK-NEXT: omp.terminator
           omp.terminator
@@ -108,16 +108,24 @@ func.func @omp_canonloop_constant_pretty() -> () {
 func.func @omp_canonloop_sequential_pretty(%tc : i32) -> () {
   // CHECK-NEXT: %canonloop_s0 = omp.new_cli
   %canonloop_s0 = omp.new_cli
-  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
-  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%tc) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop_s0) %iv_s0 : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop_s0) %iv_s0 : i32 in range(%tc) {
     // CHECK-NEXT: omp.terminator
     omp.terminator
   }
 
   // CHECK: %canonloop_s1 = omp.new_cli
   %canonloop_s1 = omp.new_cli
-  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s1) %iv : i32 in range(%[[tc]]) {
-  omp.canonical_loop(%canonloop_s1) %iv_0 : i32 in range(%tc) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop_s1) %iv_s1 : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop_s1) %iv_s1 : i32 in range(%tc) {
+    // CHECK-NEXT: omp.terminator
+    omp.terminator
+  }
+
+  // CHECK: %canonloop_s2 = omp.new_cli
+  %canonloop_s2 = omp.new_cli
+  // CHECK-NEXT: omp.canonical_loop(%canonloop_s2) %iv_s2 : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop_s2) %iv_s2 : i32 in range(%tc) {
     // CHECK-NEXT: omp.terminator
     omp.terminator
   }
@@ -126,17 +134,17 @@ func.func @omp_canonloop_sequential_pretty(%tc : i32) -> () {
 }
 
 
-// CHECK-LABEL: @omp_canonloop_nested_pretty(
+// CHECK-LABEL: @omp_canonloop_2d_nested_pretty(
 // CHECK-SAME: %[[tc:.+]]: i32)
-func.func @omp_canonloop_nested_pretty(%tc : i32) -> () {
-  // CHECK-NEXT: %canonloop_s0 = omp.new_cli
-  %canonloop_s0 = omp.new_cli
-  // CHECK-NEXT: %canonloop_s0_s0 = omp.new_cli
-  %canonloop_s0_s0 = omp.new_cli
-  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
-  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%tc) {
-    // CHECK-NEXT: omp.canonical_loop(%canonloop_s0_s0) %iv_0 : i32 in range(%[[tc]]) {
-    omp.canonical_loop(%canonloop_s0_s0) %iv_0 : i32 in range(%tc) {
+func.func @omp_canonloop_2d_nested_pretty(%tc : i32) -> () {
+  // CHECK-NEXT: %canonloop = omp.new_cli
+  %canonloop = omp.new_cli
+  // CHECK-NEXT: %canonloop_d1 = omp.new_cli
+  %canonloop_d1 = omp.new_cli
+  // CHECK-NEXT: omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop) %iv : i32 in range(%tc) {
+    // CHECK-NEXT: omp.canonical_loop(%canonloop_d1) %iv_d1 : i32 in range(%[[tc]]) {
+    omp.canonical_loop(%canonloop_d1) %iv_d1 : i32 in range(%tc) {
       // CHECK: omp.terminator
       omp.terminator
     }
@@ -147,6 +155,77 @@ func.func @omp_canonloop_nested_pretty(%tc : i32) -> () {
 }
 
 
+// CHECK-LABEL: @omp_canonloop_3d_nested_pretty(
+// CHECK-SAME: %[[tc:.+]]: i32)
+func.func @omp_canonloop_3d_nested_pretty(%tc : i32) -> () {
+  // CHECK: %canonloop = omp.new_cli
+  %canonloop = omp.new_cli
+  // CHECK: %canonloop_d1 = omp.new_cli
+  %canonloop_d1 = omp.new_cli
+  // CHECK: %canonloop_d2 = omp.new_cli
+  %canonloop_d2 = omp.new_cli
+  // CHECK-NEXT: omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop) %iv : i32 in range(%tc) {
+    // CHECK-NEXT: omp.canonical_loop(%canonloop_d1) %iv_d1 : i32 in range(%[[tc]]) {
+    omp.canonical_loop(%canonloop_d1) %iv_1d : i32 in range(%tc) {
+      // CHECK-NEXT: omp.canonical_loop(%canonloop_d2) %iv_d2 : i32 in range(%[[tc]]) {
+      omp.canonical_loop(%canonloop_d2) %iv_d2 : i32 in range(%tc) {
+        // CHECK-NEXT: omp.terminator
+        omp.terminator
+      // CHECK-NEXT: }
+      }
+      // CHECK-NEXT: omp.terminator
+      omp.terminator
+    // CHECK-NEXT: }
+    }
+    // CHECK-NEXT: omp.terminator
+    omp.terminator
+  }
+
+  return
+}
+
+
+// CHECK-LABEL: @omp_canonloop_sequential_nested_pretty(
+// CHECK-SAME: %[[tc:.+]]: i32)
+func.func @omp_canonloop_sequential_nested_pretty(%tc : i32) -> () {
+  // CHECK-NEXT: %canonloop_s0 = omp.new_cli
+  %canonloop_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop_s0_d1 = omp.new_cli
+  %canonloop_s0_d1 = omp.new_cli
+  // CHECK-NEXT: omp.canonical_loop(%canonloop_s0) %iv_s0 : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop_s0) %iv_s0 : i32 in range(%tc) {
+   // CHECK-NEXT: omp.canonical_loop(%canonloop_s0_d1) %iv_s0_d1 : i32 in range(%[[tc]]) {
+    omp.canonical_loop(%canonloop_s0_d1) %iv_s0_d1 : i32 in range(%tc) {
+      // CHECK-NEXT: omp.terminator
+      omp.terminator
+    // CHECK-NEXT: }
+    }
+    // CHECK-NEXT: omp.terminator
+    omp.terminator
+  // CHECK-NEXT: }
+  }
+
+  // CHECK-NEXT: %canonloop_s1 = omp.new_cli
+  %canonloop_s1 = omp.new_cli
+  // CHECK-NEXT: %canonloop_s1_d1 = omp.new_cli
+  %canonloop_s1_d1 = omp.new_cli
+  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s1) %iv_s1 : i32 in range(%[[tc]]) {
+  omp.canonical_loop(%canonloop_s1) %iv_s1 : i32 in range(%tc) {
+    // CHECK-NEXT:  omp.canonical_loop(%canonloop_s1_d1) %iv_s1_d1 : i32 in range(%[[tc]]) {
+    omp.canonical_loop(%canonloop_s1_d1) %iv_s1d1 : i32 in range(%tc) {
+      // CHECK-NEXT: omp.terminator
+      omp.terminator
+    // CHECK-NEXT: }
+    }
+    // CHECK-NEXT: omp.terminator
+    omp.terminator
+  }
+
+  return
+}
+
+
 // CHECK-LABEL: @omp_newcli_unused(
 // CHECK-SAME: )
 func.func @omp_newcli_unused() -> () {
diff --git a/mlir/test/Dialect/OpenMP/cli-unroll-heuristic.mlir b/mlir/test/Dialect/OpenMP/cli-unroll-heuristic.mlir
index cda7d0b500166..16884f4245e76 100644
--- a/mlir/test/Dialect/OpenMP/cli-unroll-heuristic.mlir
+++ b/mlir/test/Dialect/OpenMP/cli-unroll-heuristic.mlir
@@ -1,18 +1,18 @@
-// RUN: mlir-opt %s            | FileCheck %s
-// RUN: mlir-opt %s | mlir-opt | FileCheck %s
+// RUN: mlir-opt %s            | FileCheck %s --enable-var-scope
+// RUN: mlir-opt %s | mlir-opt | FileCheck %s --enable-var-scope
 
 
 // CHECK-LABEL: @omp_unroll_heuristic_raw(
 // CHECK-SAME: %[[tc:.+]]: i32) {
 func.func @omp_unroll_heuristic_raw(%tc : i32) -> () {
-  // CHECK-NEXT: %canonloop_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop = omp.new_cli
   %canonloop = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT: omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc]]) {
   "omp.canonical_loop" (%tc, %canonloop) ({
     ^bb0(%iv: i32):
       omp.terminator
   }) : (i32, !omp.cli) -> ()
-  // CHECK: omp.unroll_heuristic(%canonloop_s0)
+  // CHECK: omp.unroll_heuristic(%canonloop)
   "omp.unroll_heuristic" (%canonloop) : (!omp.cli) -> ()
   return
 }
@@ -22,12 +22,12 @@ func.func @omp_unroll_heuristic_raw(%tc : i32) -> () {
 // CHECK-SAME: %[[tc:.+]]: i32) {
 func.func @omp_unroll_heuristic_pretty(%tc : i32) -> () {
   // CHECK-NEXT: %[[CANONLOOP:.+]] = omp.new_cli
-  %canonloop = "omp.new_cli" () : () -> (!omp.cli)
-  // CHECK-NEXT:  omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
+  %canonloop = omp.new_cli
+  // CHECK-NEXT:  omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc]]) {
   omp.canonical_loop(%canonloop) %iv : i32 in range(%tc) {
     omp.terminator
   }
-  // CHECK: omp.unroll_heuristic(%canonloop_s0)
+  // CHECK: omp.unroll_heuristic(%canonloop)
   omp.unroll_heuristic(%canonloop)
   return
 }
@@ -36,13 +36,13 @@ func.func @omp_unroll_heuristic_pretty(%tc : i32) -> () {
 // CHECK-LABEL: @omp_unroll_heuristic_nested_pretty(
 // CHECK-SAME: %[[tc:.+]]: i32) {
 func.func @omp_unroll_heuristic_nested_pretty(%tc : i32) -> () {
-  // CHECK-NEXT: %canonloop_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop = omp.new_cli
   %cli_outer = omp.new_cli
-  // CHECK-NEXT: %canonloop_s0_s0 = omp.new_cli
+  // CHECK-NEXT: %canonloop_d1 = omp.new_cli
   %cli_inner = omp.new_cli
-  // CHECK-NEXT: omp.canonical_loop(%canonloop_s0) %iv : i32 in range(%[[tc]]) {
+  // CHECK-NEXT: omp.canonical_loop(%canonloop) %iv : i32 in range(%[[tc]]) {
   omp.canonical_loop(%cli_outer) %iv_outer : i32 in range(%tc) {
-    // CHECK-NEXT: omp.canonical_loop(%canonloop_s0_s0) %iv_0 : i32 in range(%[[tc]]) {
+    // CHECK-NEXT: omp.canonical_loop(%canonloop_d1) %iv_d1 : i32 in range(%[[tc]]) {
     omp.canonical_loop(%cli_inner) %iv_inner : i32 in range(%tc) {
       // CHECK: omp.terminator
       omp.terminator
@@ -51,9 +51,9 @@ func.func @omp_unroll_heuristic_nested_pretty(%tc : i32) -> () {
     omp.terminator
   }
 
-  // CHECK: omp.unroll_heuristic(%canonloop_s0)
+  // CHECK: omp.unroll_heuristic(%canonloop)
   omp.unroll_heuristic(%cli_outer)
-  // CHECK-NEXT: omp.unroll_heuristic(%canonloop_s0_s0)
+  // CHECK-NEXT: omp.unroll_heuristic(%canonloop_d1)
   omp.unroll_heuristic(%cli_inner)
   return
 }

tblah

It would be good to try testing the _r names as well, even if you have to (ab)use an unrelated operation to do it.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

Meinersbur · 2025-09-25T15:14:45Z

It would be good to try testing the _r names as well, even if you have to (ab)use an unrelated operation to do it.

Do you have a suggestion which operation? Need to be an operation that takes at least two region arguments.

tblah · 2025-09-26T09:31:50Z

Thanks for the replies.

Do you have a suggestion which operation? Need to be an operation that takes at least two region arguments.

The reduction and privatisation declaration operations have multiple regions. I can't imagine why we would ever generate canonical loops inside of them but it should work for testing this.

Meinersbur · 2025-09-30T09:33:37Z

The reduction and privatisation declaration operations have multiple regions.

They are "IsolatedFromAbove" though, meaning each indivually have they separated namespace.

This also meant that I previously misunderstool the "IsolatedFromAbove" trait, which I though was about the operation itself, not its region argument. I found out by trying to create a naming conflict of variable names in different region arguments. They do not conflict, so no reason to append an _r<idx> suffix. I did not find a operation with multiple region arguments in either the LLVMIR nor OpenMP dialects. There is one in FIR: fir.if. I added a test into flang as well to use it.

tblah · 2025-09-30T09:58:57Z

flang/test/Fir/omp-cli.fir

@@ -0,0 +1,28 @@
+// RUN: fir-opt %s           | FileCheck %s --enable-var-scope


Maybe this test belongs in the MLIR openmp dialect rather than in flang? It could be written with scf.if instead of fir.if etc.

In theory at least, the MLIR openmp dialect exists upstream of flang and could have other users.

I am worried about the dependencies. With FIR we can assume that the omp dialect is available, there are other tests using the omp dialect already. The openmp dialect does not require scf, nor does scf require omp.

Updated using scf.if

tblah

Thank you for the updates

Improve the automatic naming of variables defined by the `omp.canonical_loop` operation: 1. The iteration variable gets a name consistent with the cli variable 2. Instead of appending `_s0` for each nesting level, shorten it to `_d<num>` for a perfectly nested loop at depth `<num>` 3. Do not add any suffix to the top-level loop if it is the only top-level loop

Meinersbur added 4 commits September 19, 2025 14:43

Improve canonloop/iv naming

b391971

Merge branch 'main' into users/meinersbur/mlir_loop-varnaming

a6e3426

Avoid compiler warning

ce66eec

Add perfect-nest and rectangular loop nest tests

3a141d6

Meinersbur changed the base branch from main to users/meinersbur/flang_loopnest-checks September 23, 2025 13:28

Meinersbur mentioned this pull request Sep 23, 2025

[Flang] Add standalone tile support #160298

Merged

Meinersbur added 2 commits September 23, 2025 16:03

Merge branch 'users/meinersbur/flang_loopnest-checks' into HEAD

375e948

Fix symbol resolution

4934424

Meinersbur changed the title ~~[mlir][OpenMP] Improve canonloop/iv naming~~ [mlir][omp] Improve canonloop/iv naming Sep 24, 2025

Merge branch 'users/meinersbur/flang_loopnest-checks' into HEAD

dd3f66d

Meinersbur marked this pull request as ready for review September 24, 2025 10:30

llvmbot added mlir mlir:openmp flang:openmp labels Sep 24, 2025

Meinersbur requested review from ergawy and tblah September 24, 2025 10:32

Meinersbur mentioned this pull request Sep 24, 2025

[mlir][omp] Add omp.tile operation #160292

Merged

Meinersbur requested a review from skatrak September 24, 2025 11:15

tblah reviewed Sep 24, 2025

View reviewed changes

Meinersbur added 4 commits September 29, 2025 14:27

Handle IsolatedFromAbove individually

b1c6e22

clarify function comment

a2b0388

isOnlyOpInRegion -> isOnlyContainerOpInRegion

3564371

rework algorithm

e410237

llvmbot added the flang Flang issues not falling into any other category label Sep 30, 2025

flatten if-else chain

fdc54a6

tblah reviewed Sep 30, 2025

View reviewed changes

Move fir test to mlir openmp using scf.if

757117d

tblah approved these changes Sep 30, 2025

View reviewed changes

Base automatically changed from users/meinersbur/flang_loopnest-checks to main September 30, 2025 10:32

Meinersbur added 2 commits October 2, 2025 13:52

Merge branch 'main' into users/meinersbur/mlir_loop-varnaming

afd9edc

Avoid use of anon struct/union

b2847e7

Meinersbur merged commit 1c1d525 into main Oct 2, 2025
9 checks passed

Meinersbur deleted the users/meinersbur/mlir_loop-varnaming branch October 2, 2025 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][omp] Improve canonloop/iv naming #159773

[mlir][omp] Improve canonloop/iv naming #159773

Uh oh!

Meinersbur commented Sep 19, 2025 •

edited

Loading

Uh oh!

llvmbot commented Sep 24, 2025 •

edited

Loading

Uh oh!

tblah left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Meinersbur commented Sep 25, 2025

Uh oh!

tblah commented Sep 26, 2025

Uh oh!

Meinersbur commented Sep 30, 2025

Uh oh!

tblah Sep 30, 2025

Uh oh!

Meinersbur Sep 30, 2025

Uh oh!

Meinersbur Sep 30, 2025

Uh oh!

tblah left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,28 @@
		// RUN: fir-opt %s \| FileCheck %s --enable-var-scope

[mlir][omp] Improve canonloop/iv naming #159773

[mlir][omp] Improve canonloop/iv naming #159773

Uh oh!

Conversation

Meinersbur commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Meinersbur commented Sep 25, 2025

Uh oh!

tblah commented Sep 26, 2025

Uh oh!

Meinersbur commented Sep 30, 2025

Uh oh!

tblah Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Meinersbur Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Meinersbur Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Meinersbur commented Sep 19, 2025 •

edited

Loading

llvmbot commented Sep 24, 2025 •

edited

Loading