[Mlir-commits] [mlir] Revert "[mlir][acc] Replace terminators with scf.yield in wrapMultiBlockRegionWithSCFExecuteRegion (#183758)" (PR #184228)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Mon Mar 2 12:58:14 PST 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-openacc
@llvm/pr-subscribers-openacc
Author: Delaram Talaashrafi (delaram-talaashrafi)
<details>
<summary>Changes</summary>
This reverts commit 12f4eb2156559c2f8c99fa7dc3b59cb4fef1389d.
https://lab.llvm.org/buildbot/#/builders/55/builds/24871.
---
Full diff: https://github.com/llvm/llvm-project/pull/184228.diff
3 Files Affected:
- (modified) mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsLoop.h (+6-10)
- (modified) mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsLoop.cpp (+13-18)
- (modified) mlir/unittests/Dialect/OpenACC/OpenACCUtilsLoopTest.cpp (-154)
``````````diff
diff --git a/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsLoop.h b/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsLoop.h
index c6c08acbc53bb..c7d73f74ce8aa 100644
--- a/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsLoop.h
+++ b/mlir/include/mlir/Dialect/OpenACC/OpenACCUtilsLoop.h
@@ -27,24 +27,20 @@ namespace acc {
class LoopOp;
/// Wrap a multi-block region in an scf.execute_region.
-/// Clones the given region into a new scf.execute_region. Replaces acc.yield
-/// with scf.yield; when convertFuncReturn is true, also replaces func.return
-/// with scf.yield. Use this to convert unstructured control flow (e.g. multiple
-/// blocks with branches) into a single SCF region.
+/// Clones the given region into a new scf.execute_region, replacing
+/// acc.yield/acc.terminator with scf.yield. Use this to convert unstructured
+/// control flow (e.g. multiple blocks with branches) into a single SCF region.
/// @param region The region to wrap (cloned into the execute_region; not
/// modified).
/// @param mapping IR mapping for the clone; updated with block and value
/// mappings.
/// @param loc Location for the created execute_region op.
/// @param rewriter RewriterBase for creating and erasing operations.
-/// @param convertFuncReturn When true, replace func.return with scf.yield in
-/// addition to acc.yield. Default is false.
-/// @return The created scf.execute_region operation, or nullptr if any replaced
-/// terminator has operands (results not yet supported).
+/// @return The created scf.execute_region operation, or nullptr if the region
+/// has an acc.yield with operands (results not yet supported).
scf::ExecuteRegionOp
wrapMultiBlockRegionWithSCFExecuteRegion(Region ®ion, IRMapping &mapping,
- Location loc, RewriterBase &rewriter,
- bool convertFuncReturn = false);
+ Location loc, RewriterBase &rewriter);
/// Convert a structured acc.loop to scf.for.
/// The loop arguments are converted to index type. If enableCollapse is true,
/// nested loops are collapsed into a single loop.
diff --git a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsLoop.cpp b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsLoop.cpp
index 37171cae342dc..b76d3f338924c 100644
--- a/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsLoop.cpp
+++ b/mlir/lib/Dialect/OpenACC/Utils/OpenACCUtilsLoop.cpp
@@ -14,7 +14,6 @@
#include "mlir/Dialect/Arith/IR/Arith.h"
#include "mlir/Dialect/Arith/Utils/Utils.h"
-#include "mlir/Dialect/Func/IR/FuncOps.h"
#include "mlir/Dialect/OpenACC/OpenACC.h"
#include "mlir/Dialect/SCF/IR/SCF.h"
#include "mlir/Dialect/SCF/Utils/Utils.h"
@@ -152,31 +151,27 @@ namespace acc {
/// Wrap a multi-block region with scf.execute_region.
scf::ExecuteRegionOp
wrapMultiBlockRegionWithSCFExecuteRegion(Region ®ion, IRMapping &mapping,
- Location loc, RewriterBase &rewriter,
- bool convertFuncReturn) {
+ Location loc, RewriterBase &rewriter) {
auto exeRegionOp = scf::ExecuteRegionOp::create(rewriter, loc, TypeRange{});
rewriter.cloneRegionBefore(region, exeRegionOp.getRegion(),
exeRegionOp.getRegion().end(), mapping);
- // Find and replace the ACC terminators with scf.yield in each block
- for (Block &block : exeRegionOp.getRegion().getBlocks()) {
- if (block.empty())
- continue;
- Operation *blockTerminator = block.getTerminator();
- if ((convertFuncReturn && isa<func::ReturnOp>(*blockTerminator)) ||
- isa<acc::YieldOp>(*blockTerminator)) {
- if (blockTerminator->getNumOperands()) {
- region.getParentOp()->emitError(
- "region with results not yet supported");
- return nullptr;
- }
- rewriter.setInsertionPointToEnd(&block);
- (void)scf::YieldOp::create(rewriter, blockTerminator->getLoc());
- blockTerminator->erase();
+ // Find and replace the ACC terminator with scf.yield
+ Operation *terminator = exeRegionOp.getRegion().back().getTerminator();
+ if (auto yieldOp = dyn_cast<acc::YieldOp>(terminator)) {
+ if (yieldOp.getNumOperands() > 0) {
+ region.getParentOp()->emitError(
+ "acc.loop with results not yet supported");
+ return nullptr;
}
+ } else if (!isa<acc::TerminatorOp>(terminator)) {
+ llvm_unreachable("unexpected terminator in ACC region");
}
+ rewriter.eraseOp(terminator);
+ rewriter.setInsertionPointToEnd(&exeRegionOp.getRegion().back());
+ scf::YieldOp::create(rewriter, loc);
return exeRegionOp;
}
diff --git a/mlir/unittests/Dialect/OpenACC/OpenACCUtilsLoopTest.cpp b/mlir/unittests/Dialect/OpenACC/OpenACCUtilsLoopTest.cpp
index 70ace8d993b20..83c162e32dedc 100644
--- a/mlir/unittests/Dialect/OpenACC/OpenACCUtilsLoopTest.cpp
+++ b/mlir/unittests/Dialect/OpenACC/OpenACCUtilsLoopTest.cpp
@@ -649,160 +649,6 @@ TEST_F(OpenACCUtilsLoopTest,
EXPECT_EQ(mulCount, 1u);
}
-TEST_F(OpenACCUtilsLoopTest,
- WrapMultiBlockWithEarlyExitRegionWithSCFExecuteRegion) {
- auto [module, funcOp] = createModuleWithFunc();
-
- OwningOpRef<acc::ParallelOp> parallelOp =
- acc::ParallelOp::create(b, loc, TypeRange{}, ValueRange{});
- Region ®ion = parallelOp->getRegion();
- // Block order as in all.mlir: ^bb0 entry, ^bb1 header, ^bb2 exit, ^bb3 body
- Block *entry = b.createBlock(®ion, region.begin());
- Block *header = b.createBlock(®ion, region.end());
- Block *exitBlock = b.createBlock(®ion, region.end());
- Block *bodyBlock = b.createBlock(®ion, region.end());
-
- // ^bb0 (entry): setup then cf.br ^bb1
- b.setInsertionPointToEnd(entry);
- Value c1 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(1));
- arith::AddIOp::create(b, loc, c1, c1); // some op like fir.store in all.mlir
- cf::BranchOp::create(b, loc, header);
-
- // ^bb1 (header): 2 preds (entry, body). cond_br to exit or body
- b.setInsertionPointToEnd(header);
- Value cond =
- arith::ConstantOp::create(b, loc, b.getI1Type(), b.getBoolAttr(false));
- cf::CondBranchOp::create(b, loc, cond, exitBlock, bodyBlock);
-
- // ^bb2 (exit): return — use acc.yield (replaced with scf.yield by the util)
- b.setInsertionPointToEnd(exitBlock);
- acc::YieldOp::create(b, loc);
-
- // ^bb3 (body): body ops then cf.br ^bb1
- b.setInsertionPointToEnd(bodyBlock);
- Value c2 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(2));
- Value c3 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(3));
- arith::MulIOp::create(b, loc, c2, c3);
- cf::BranchOp::create(b, loc, header);
-
- EXPECT_EQ(region.getBlocks().size(), 4u);
-
- b.setInsertionPointAfter(parallelOp.get());
- IRMapping mapping;
- scf::ExecuteRegionOp exeRegionOp =
- wrapMultiBlockRegionWithSCFExecuteRegion(region, mapping, loc, b);
-
- ASSERT_TRUE(exeRegionOp);
-
- EXPECT_EQ(exeRegionOp.getRegion().getBlocks().size(), 4u);
-
- // First block (entry): branch to header
- Block &exeEntry = exeRegionOp.getRegion().front();
- EXPECT_TRUE(isa<cf::BranchOp>(exeEntry.getTerminator()));
-
- // Second block (header): cond_br, 2 predecessors (entry and body)
- Block &exeHeader = *std::next(exeRegionOp.getRegion().begin());
- EXPECT_TRUE(isa<cf::CondBranchOp>(exeHeader.getTerminator()));
-
- // Third block (exit): scf.yield
- Block &exeExit = *std::next(exeRegionOp.getRegion().begin(), 2);
- EXPECT_TRUE(isa<scf::YieldOp>(exeExit.getTerminator()));
-
- // Fourth block (body): branch back to header
- Block &exeBody = exeRegionOp.getRegion().back();
- EXPECT_TRUE(isa<cf::BranchOp>(exeBody.getTerminator()));
- EXPECT_EQ(exeBody.getTerminator()->getSuccessor(0), &exeHeader);
-
- // Body ops preserved: one addi (entry), one muli (body)
- unsigned addCount = 0;
- unsigned mulCount = 0;
- exeRegionOp.getRegion().walk([&](arith::AddIOp) { ++addCount; });
- exeRegionOp.getRegion().walk([&](arith::MulIOp) { ++mulCount; });
- EXPECT_EQ(addCount, 1u);
- EXPECT_EQ(mulCount, 1u);
-}
-
-TEST_F(OpenACCUtilsLoopTest,
- WrapMultiBlockRegionWithSCFExecuteRegionConvertFuncReturn) {
- auto [module, funcOp] = createModuleWithFunc();
-
- // Build a multi-block region that uses func.return (function body).
- // wrapMultiBlockRegionWithSCFExecuteRegion(..., true) should replace
- // func.return with scf.yield.
- Region ®ion = funcOp.getBody();
- Block *entry = ®ion.front();
- Block *thenBlock = b.createBlock(®ion, region.end());
- Block *elseBlock = b.createBlock(®ion, region.end());
- Block *exitBlock = b.createBlock(®ion, region.end());
- Block *tailBlock = b.createBlock(®ion, region.end());
-
- b.setInsertionPointToEnd(entry);
- Value cond =
- arith::ConstantOp::create(b, loc, b.getI1Type(), b.getBoolAttr(true));
- cf::CondBranchOp::create(b, loc, cond, thenBlock, elseBlock);
-
- b.setInsertionPointToEnd(thenBlock);
- Value c1 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(1));
- Value c2 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(2));
- arith::AddIOp::create(b, loc, c1, c2);
- cf::BranchOp::create(b, loc, exitBlock);
-
- b.setInsertionPointToEnd(elseBlock);
- Value c3 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(3));
- Value c4 =
- arith::ConstantOp::create(b, loc, b.getIndexType(), b.getIndexAttr(4));
- arith::MulIOp::create(b, loc, c3, c4);
- cf::BranchOp::create(b, loc, exitBlock);
-
- b.setInsertionPointToEnd(exitBlock);
- func::ReturnOp::create(b, loc, ValueRange{});
-
- b.setInsertionPointToEnd(tailBlock);
- cf::BranchOp::create(b, loc, exitBlock);
-
- EXPECT_EQ(region.getBlocks().size(), 5u);
-
- b.setInsertionPointAfter(funcOp);
- IRMapping mapping;
- scf::ExecuteRegionOp exeRegionOp = wrapMultiBlockRegionWithSCFExecuteRegion(
- region, mapping, loc, b, /*convertFuncReturn=*/true);
-
- ASSERT_TRUE(exeRegionOp);
-
- EXPECT_EQ(exeRegionOp.getRegion().getBlocks().size(), 5u);
-
- // Only the block that had func.return is replaced with scf.yield; other
- // blocks keep their cf.cond_br / cf.br terminators.
- unsigned exeBlockIndex = 0;
- Block *exeExitBlock = nullptr;
- for (Block &block : exeRegionOp.getRegion().getBlocks()) {
- if (exeBlockIndex ==
- 3) // exitBlock (entry=0, then=1, else=2, exit=3, tail=4)
- exeExitBlock = █
- exeBlockIndex++;
- }
- ASSERT_TRUE(exeExitBlock);
- EXPECT_TRUE(isa<scf::YieldOp>(exeExitBlock->getTerminator()))
- << "block that had func.return should now have scf.yield";
-
- // Control flow and body ops preserved
- Block &exeEntry = exeRegionOp.getRegion().front();
- EXPECT_TRUE(isa<cf::CondBranchOp>(exeEntry.getTerminator()));
-
- unsigned addCount = 0;
- unsigned mulCount = 0;
- exeRegionOp.getRegion().walk([&](arith::AddIOp) { ++addCount; });
- exeRegionOp.getRegion().walk([&](arith::MulIOp) { ++mulCount; });
- EXPECT_EQ(addCount, 1u);
- EXPECT_EQ(mulCount, 1u);
-}
-
//===----------------------------------------------------------------------===//
// Error Case Tests
//===----------------------------------------------------------------------===//
``````````
</details>
https://github.com/llvm/llvm-project/pull/184228
More information about the Mlir-commits
mailing list