https://github.com/Meinersbur updated https://github.com/llvm/llvm-project/pull/159773
>From b3919715ebe223b39dd789dcd471a864666d7008 Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Fri, 19 Sep 2025 14:43:48 +0200 Subject: [PATCH 1/6] Improve canonloop/iv naming --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 237 +++++++++++++----- .../Dialect/OpenMP/cli-canonical_loop.mlir | 127 ++++++++-- .../Dialect/OpenMP/cli-unroll-heuristic.mlir | 28 +-- 3 files changed, 292 insertions(+), 100 deletions(-) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 3d70e28ed23ab..cf549a6bb50b4 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -77,6 +77,178 @@ 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 (®ion == 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 (auto &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 + int curNestRoot = 0; + int curNestDepth = 1; + auto mergeLoopNest = [&](int innermost) { + auto 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 +3313,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 (®ion == 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 +3404,8 @@ void CanonicalLoopOp::getAsmBlockNames(OpAsmSetBlockNameFn setNameFn) { void CanonicalLoopOp::getAsmBlockArgumentNames(Region ®ion, 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 } >From ce66eec648f6415c199d6115f3be4d188eee59ba Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Tue, 23 Sep 2025 12:19:41 +0200 Subject: [PATCH 2/6] Avoid compiler warning --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index cf549a6bb50b4..1674891410194 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -170,22 +170,21 @@ static std::string generateLoopNestingName(StringRef prefix, std::reverse(components.begin(), components.end()); // Determine whether a component is not needed - for (auto &c : components) { + 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 - int curNestRoot = 0; - int curNestDepth = 1; - auto mergeLoopNest = [&](int innermost) { - auto outermost = curNestRoot; + 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)) { + for (auto i : llvm::seq<int>(outermost + 1, innermost)) components[i].skipOp = true; - } components[innermost].depth = curNestDepth; } >From b1c6e22532c6103c1cc9153e2f0036a438b482ad Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Mon, 29 Sep 2025 14:27:45 +0200 Subject: [PATCH 3/6] Handle IsolatedFromAbove individually --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 15 ++++--- .../Dialect/OpenMP/cli-canonical_loop.mlir | 44 +++++++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 1674891410194..44ef943b63335 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -106,9 +106,6 @@ static std::string generateLoopNestingName(StringRef prefix, // 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) @@ -147,7 +144,14 @@ static std::string generateLoopNestingName(StringRef prefix, comp.parentOp = parent; comp.regionInOpIdx = 0; comp.isOnlyRegionInOp = true; - if (parent && parent->getRegions().size() > 1) { + + // Need to disambiguate between different region arguments? The + // IsolatedFromAbove trait of the parent operation implies that each + // individual region argument has its own separate namespace. + if (!parent || parent->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>()) + break; + + if (parent->getRegions().size() > 1) { auto getRegionIndex = [](Operation *o, Region *r) { for (auto [idx, region] : llvm::enumerate(o->getRegions())) { if (®ion == r) @@ -159,9 +163,6 @@ static std::string generateLoopNestingName(StringRef prefix, comp.isOnlyRegionInOp = false; } - if (!parent) - break; - // next parent o = parent; } diff --git a/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir b/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir index 874e3922805ec..c77253cea94fe 100644 --- a/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir +++ b/mlir/test/Dialect/OpenMP/cli-canonical_loop.mlir @@ -234,3 +234,47 @@ func.func @omp_newcli_unused() -> () { // CHECK-NEXT: return return } + + +// CHECK-LABEL: @omp_canonloop_multiregion_isolatedfromabove( +func.func @omp_canonloop_multiregion_isolatedfromabove() -> () { + omp.private {type = firstprivate} @x.privatizer : !llvm.ptr init { + ^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr): + %c42_i32 = arith.constant 42: i32 + // CHECK: omp.canonical_loop %iv : i32 in range(%c42_i32) { + omp.canonical_loop %iv1 : i32 in range(%c42_i32) { + omp.terminator + } + // CHECK: omp.yield + omp.yield(%arg0 : !llvm.ptr) + } copy { + ^bb0(%arg0: !llvm.ptr, %arg1: !llvm.ptr): + %c42_i32 = arith.constant 42: i32 + // CHECK: omp.canonical_loop %iv : i32 in range(%c42_i32) { + omp.canonical_loop %iv : i32 in range(%c42_i32) { + // CHECK: omp.canonical_loop %iv_d1 : i32 in range(%c42_i32) { + omp.canonical_loop %iv_d1 : i32 in range(%c42_i32) { + omp.terminator + } + omp.terminator + } + // CHECK: omp.yield + omp.yield(%arg0 : !llvm.ptr) + } dealloc { + ^bb0(%arg0: !llvm.ptr): + %c42_i32 = arith.constant 42: i32 + // CHECK: omp.canonical_loop %iv_s0 : i32 in range(%c42_i32) { + omp.canonical_loop %iv_s0 : i32 in range(%c42_i32) { + omp.terminator + } + // CHECK: omp.canonical_loop %iv_s1 : i32 in range(%c42_i32) { + omp.canonical_loop %iv_s1 : i32 in range(%c42_i32) { + omp.terminator + } + // CHECK: omp.yield + omp.yield + } + + // CHECK: return + return +} >From a2b0388ebf3a4c21dec7ba294e01d817d728d6ce Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Mon, 29 Sep 2025 14:42:21 +0200 Subject: [PATCH 4/6] clarify function comment --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 44ef943b63335..fbb1161097a05 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -78,11 +78,19 @@ 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. +/// `<prefix>(_r<idx>_s<idx>)*`. Hereby, `_r<idx>` identifies the region +/// argument index of an operation that has multiple regions, if the operation +/// has multiple regions. +/// `_s<idx>` identifies the position of an operation within a region, where +/// only operations that may potentially contain loops (i.e. have region +/// arguments) are counted. Again, it is omitted if there is only one such +/// operation in a region. If there are canonical loops nested inside each +/// other, also may also use the format `_d<num>` where <num> is the nesting +/// depth of the loop. +/// +/// The generated name is a best-effort to make canonical loop unique within an +/// SSA namespace. This also means that regions with IsolatedFromAbove property +/// do not consider any parents or siblings. static std::string generateLoopNestingName(StringRef prefix, CanonicalLoopOp op) { struct Component { >From 3564371e1d2bc4ffcaedbb54038d69630c7a24f7 Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Mon, 29 Sep 2025 14:47:58 +0200 Subject: [PATCH 5/6] isOnlyOpInRegion -> isOnlyContainerOpInRegion --- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 36 ++++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index fbb1161097a05..15453631b2f59 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -82,7 +82,7 @@ struct LLVMPointerPointerLikeModel /// argument index of an operation that has multiple regions, if the operation /// has multiple regions. /// `_s<idx>` identifies the position of an operation within a region, where -/// only operations that may potentially contain loops (i.e. have region +/// only operations that may potentially contain loops ("container operations" i.e. have region /// arguments) are counted. Again, it is omitted if there is only one such /// operation in a region. If there are canonical loops nested inside each /// other, also may also use the format `_d<num>` where <num> is the nesting @@ -100,12 +100,12 @@ static std::string generateLoopNestingName(StringRef prefix, bool isOnlyRegionInOp; bool skipRegion; - // An operation somewhere in a parent region - Operation *thisOp; + // An container operation somewhere in a parent region + Operation *containerOp; Region *parentRegion; - size_t opInRegionIdx; - bool isOnlyOpInRegion; - bool skipOp; + size_t containerOpInRegionIdx; + bool isOnlyContainerOpInRegion; + bool skipContainerOp; int depth = -1; }; SmallVector<Component> components; @@ -141,10 +141,10 @@ static std::string generateLoopNestingName(StringRef prefix, } Component &comp = components.emplace_back(); - comp.thisOp = o; + comp.containerOp = o; comp.parentRegion = r; - comp.opInRegionIdx = sequentialIdx; - comp.isOnlyOpInRegion = isOnlyLoop; + comp.containerOpInRegionIdx = sequentialIdx; + comp.isOnlyContainerOpInRegion = isOnlyLoop; // Region argument of an operation Operation *parent = r->getParentOp(); @@ -181,7 +181,7 @@ static std::string generateLoopNestingName(StringRef prefix, // Determine whether a component is not needed for (Component &c : components) { c.skipRegion = c.isOnlyRegionInOp; - c.skipOp = c.isOnlyOpInRegion && !isa<CanonicalLoopOp>(c.thisOp); + c.skipContainerOp = c.isOnlyContainerOpInRegion && !isa<CanonicalLoopOp>(c.containerOp); } // Find runs of perfect nests and merge them into a single component @@ -193,7 +193,7 @@ static std::string generateLoopNestingName(StringRef prefix, // 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[i].skipContainerOp = true; components[innermost].depth = curNestDepth; } @@ -211,10 +211,10 @@ static std::string generateLoopNestingName(StringRef prefix, continue; } - if (c.skipOp) + if (c.skipContainerOp) continue; - if (!c.isOnlyOpInRegion) { + if (!c.isOnlyContainerOpInRegion) { mergeLoopNest(i); continue; } @@ -227,10 +227,10 @@ static std::string generateLoopNestingName(StringRef prefix, // Outermost loop does not need a suffix if it has no sibling for (auto &c : components) { - if (c.skipOp) + if (c.skipContainerOp) continue; - if (c.isOnlyOpInRegion) - c.skipOp = true; + if (c.isOnlyContainerOpInRegion) + c.skipContainerOp = true; break; } @@ -246,11 +246,11 @@ static std::string generateLoopNestingName(StringRef prefix, if (!c.skipRegion) addComponent('r', c.regionInOpIdx); - if (!c.skipOp) { + if (!c.skipContainerOp) { if (c.depth >= 0) addComponent('d', c.depth - 1); else - addComponent('s', c.opInRegionIdx); + addComponent('s', c.containerOpInRegionIdx); } } >From e41023737571e742dd5ad82304692a6f2c235ef1 Mon Sep 17 00:00:00 2001 From: Michael Kruse <[email protected]> Date: Mon, 29 Sep 2025 17:41:17 +0200 Subject: [PATCH 6/6] rework algorithm --- flang/test/Fir/omp-cli.fir | 28 +++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | 218 ++++++++++--------- 2 files changed, 149 insertions(+), 97 deletions(-) create mode 100644 flang/test/Fir/omp-cli.fir diff --git a/flang/test/Fir/omp-cli.fir b/flang/test/Fir/omp-cli.fir new file mode 100644 index 0000000000000..8e5ea00b9744a --- /dev/null +++ b/flang/test/Fir/omp-cli.fir @@ -0,0 +1,28 @@ +// RUN: fir-opt %s | FileCheck %s --enable-var-scope +// RUN: fir-opt %s | fir-opt | FileCheck %s --enable-var-scope + +// CHECK-LABEL: @omp_canonloop_multiregion( +func.func @omp_canonloop_multiregion(%c : i1) -> () { + %c42_i32 = arith.constant 42: i32 + %canonloop1 = omp.new_cli + %canonloop2 = omp.new_cli + %canonloop3 = omp.new_cli + fir.if %c { + // CHECK: omp.canonical_loop(%canonloop_r0) %iv_r0 : i32 in range(%c42_i32) { + omp.canonical_loop(%canonloop1) %iv1 : i32 in range(%c42_i32) { + omp.terminator + } + } else { + // CHECK: omp.canonical_loop(%canonloop_r1_s0) %iv_r1_s0 : i32 in range(%c42_i32) { + omp.canonical_loop(%canonloop2) %iv2 : i32 in range(%c42_i32) { + omp.terminator + } + // CHECK: omp.canonical_loop(%canonloop_r1_s1) %iv_r1_s1 : i32 in range(%c42_i32) { + omp.canonical_loop(%canonloop3) %iv3 : i32 in range(%c42_i32) { + omp.terminator + } + } + + // CHECK: fir.unreachable + fir.unreachable +} diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp index 15453631b2f59..5d4039ba37b83 100644 --- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp +++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp @@ -82,11 +82,11 @@ struct LLVMPointerPointerLikeModel /// argument index of an operation that has multiple regions, if the operation /// has multiple regions. /// `_s<idx>` identifies the position of an operation within a region, where -/// only operations that may potentially contain loops ("container operations" i.e. have region -/// arguments) are counted. Again, it is omitted if there is only one such -/// operation in a region. If there are canonical loops nested inside each -/// other, also may also use the format `_d<num>` where <num> is the nesting -/// depth of the loop. +/// only operations that may potentially contain loops ("container operations" +/// i.e. have region arguments) are counted. Again, it is omitted if there is +/// only one such operation in a region. If there are canonical loops nested +/// inside each other, also may also use the format `_d<num>` where <num> is the +/// nesting depth of the loop. /// /// The generated name is a best-effort to make canonical loop unique within an /// SSA namespace. This also means that regions with IsolatedFromAbove property @@ -94,20 +94,31 @@ struct LLVMPointerPointerLikeModel 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 container operation somewhere in a parent region - Operation *containerOp; - Region *parentRegion; - size_t containerOpInRegionIdx; - bool isOnlyContainerOpInRegion; - bool skipContainerOp; - int depth = -1; + bool isRegionArgOfOp; + bool skip = false; + bool isUnique = false; + size_t idx; + + union { + /// isRegionArgOfOp == true: region argument of an operation + struct { + Operation *ownerOp; + }; + /// isRegionArgOfOp == false: container op in a region + struct { + Operation *containerOp; + Region *parentRegion; + size_t loopDepth; + }; + }; + + void skipIf(bool v = true) { skip = skip || v; } }; + + // List of ancestors, from inner to outer. + // Alternates between + // * region argument of an operation + // * operation within a region SmallVector<Component> components; // Gather a list of parent regions and operations, and the position within @@ -123,7 +134,7 @@ static std::string generateLoopNestingName(StringRef prefix, size_t idx = 0; bool found = false; size_t sequentialIdx = -1; - bool isOnlyLoop = true; + bool isOnlyContainerOp = true; for (Block *b : traversal) { for (Operation &op : *b) { if (&op == o && !found) { @@ -133,32 +144,37 @@ static std::string generateLoopNestingName(StringRef prefix, if (op.getNumRegions()) { idx += 1; if (idx > 1) - isOnlyLoop = false; + isOnlyContainerOp = false; } - if (found && !isOnlyLoop) + if (found && !isOnlyContainerOp) break; } } - Component &comp = components.emplace_back(); - comp.containerOp = o; - comp.parentRegion = r; - comp.containerOpInRegionIdx = sequentialIdx; - comp.isOnlyContainerOpInRegion = isOnlyLoop; + Component &containerOpInRegion = components.emplace_back(); + containerOpInRegion.isRegionArgOfOp = false; + containerOpInRegion.containerOp = o; + containerOpInRegion.parentRegion = r; + containerOpInRegion.idx = sequentialIdx; + containerOpInRegion.isUnique = isOnlyContainerOp; - // Region argument of an operation Operation *parent = r->getParentOp(); - comp.parentOp = parent; - comp.regionInOpIdx = 0; - comp.isOnlyRegionInOp = true; - - // Need to disambiguate between different region arguments? The - // IsolatedFromAbove trait of the parent operation implies that each - // individual region argument has its own separate namespace. + // Region argument of an operation + Component ®ionArgOfOperation = components.emplace_back(); + regionArgOfOperation.isRegionArgOfOp = true; + regionArgOfOperation.ownerOp = parent; + regionArgOfOperation.idx = 0; + regionArgOfOperation.isUnique = true; + + // The IsolatedFromAbove trait of the parent operation implies that each + // individual region argument has its own separate namespace, so no + // ambiguity. if (!parent || parent->hasTrait<mlir::OpTrait::IsIsolatedFromAbove>()) break; + // Component only needed if operation has multiple region operands. Region + // arguments may be optional, but we currently do not consider this. if (parent->getRegions().size() > 1) { auto getRegionIndex = [](Operation *o, Region *r) { for (auto [idx, region] : llvm::enumerate(o->getRegions())) { @@ -167,94 +183,102 @@ static std::string generateLoopNestingName(StringRef prefix, } llvm_unreachable("Region not child of its parent operation"); }; - comp.regionInOpIdx = getRegionIndex(parent, r); - comp.isOnlyRegionInOp = false; + regionArgOfOperation.idx = getRegionIndex(parent, r); + regionArgOfOperation.isUnique = false; } // 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.skipContainerOp = c.isOnlyContainerOpInRegion && !isa<CanonicalLoopOp>(c.containerOp); - } + // Determine whether a region-argument component is not needed + for (Component &c : components) + c.skipIf(c.isRegionArgOfOp && c.isUnique); - // 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].skipContainerOp = true; - components[innermost].depth = curNestDepth; - } - - // Start new root - curNestRoot = innermost + 1; - curNestDepth = 1; - }; - for (auto &&[i, c] : llvm::enumerate(components)) { - if (i <= curNestRoot) + // Find runs of nested loops and determine each loop's depth in the loop nest + size_t numSurroundingLoops = 0; + for (Component &c : llvm::reverse(components)) { + if (c.skip) continue; - // Check whether this region can be included - if (!c.skipRegion) { - mergeLoopNest(i); + // non-skipped multi-argument operands interrupt the loop nest + if (c.isRegionArgOfOp) { + numSurroundingLoops = 0; continue; } - if (c.skipContainerOp) - continue; + // Multiple loops in a region means each of them is the outermost loop of a + // new loop nest + if (!c.isUnique) + numSurroundingLoops = 0; - if (!c.isOnlyContainerOpInRegion) { - mergeLoopNest(i); - continue; - } + c.loopDepth = numSurroundingLoops; - curNestDepth += 1; + // Next loop is surrounded by one more loop + if (isa<CanonicalLoopOp>(c.containerOp)) + numSurroundingLoops += 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.skipContainerOp) + // In loop nests, skip all but the innermost loop that contains the depth + // number + bool isLoopNest = false; + for (Component &c : components) { + if (c.skip || c.isRegionArgOfOp) continue; - if (c.isOnlyContainerOpInRegion) - c.skipContainerOp = true; - break; + + if (!isLoopNest && c.loopDepth >= 1) { + // Innermost loop of a loop nest of at least two loops + isLoopNest = true; + } else if (isLoopNest) { + // Non-innermost loop of a loop nest + c.skipIf(c.isUnique); + + // If there is no surrounding loop left, this must have been the outermost + // loop; leave loop-nest mode for the next iteration + if (c.loopDepth == 0) + isLoopNest = false; + } } - // Compile name - SmallString<64> Name{prefix}; - for (auto &c : components) { - auto addComponent = [&Name](char letter, int64_t idx) { - Name += '_'; - Name += letter; - Name += idx; - }; + // Skip non-loop unambiguous regions (but they should interrupt loop nests, so + // we mark them as skipped only after computing loop nests) + for (Component &c : components) + c.skipIf(!c.isRegionArgOfOp && c.isUnique && + !isa<CanonicalLoopOp>(c.containerOp)); + + // Components can be skipped if they are already disambiguated by their parent + // (or does not have a parent) + bool newRegion = true; + for (Component &c : llvm::reverse(components)) { + c.skipIf(newRegion && c.isUnique); + + // non-skipped components disambiguate unique children + if (!c.skip) + newRegion = true; + + // ...except canonical loops that need a suffix for each nest + if (!c.isRegionArgOfOp && isa<CanonicalLoopOp>(c.containerOp)) + newRegion = false; + } - if (!c.skipRegion) - addComponent('r', c.regionInOpIdx); + // Compile the nesting name string + SmallString<64> Name{prefix}; + llvm::raw_svector_ostream NameOS(Name); + for (auto &c : llvm::reverse(components)) { + if (c.skip) + continue; - if (!c.skipContainerOp) { - if (c.depth >= 0) - addComponent('d', c.depth - 1); + if (c.isRegionArgOfOp) { + NameOS << "_r" << c.idx; + } else { + if (c.loopDepth >= 1) + NameOS << "_d" << c.loopDepth; else - addComponent('s', c.containerOpInRegionIdx); + NameOS << "_s" << c.idx; } } - return Name.str().str(); + return NameOS.str().str(); } void OpenMPDialect::initialize() { _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
