[Mlir-commits] [mlir] [mlir][acc] Don't strip a live op's implicit `acc.terminator` during host fallback (PR #205731)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Wed Jun 24 23:25:13 PDT 2026
https://github.com/khaki3 created https://github.com/llvm/llvm-project/pull/205731
Example:
```fortran
!$acc parallel if(cond)
!$acc atomic capture
a = a + 1
b = a
!$acc end atomic
!$acc end parallel
```
In this code, the `if` clause triggers host-fallback specialization. A blanket `acc.terminator` erase pattern can strip the implicit terminator of the still-present `acc.atomic.capture`, so the later `getTerminator()` trips `mightHaveTerminator()`.
Fix: only erase `acc.terminator` once it's no longer owned by a live ACC region op (owning ops erase their own when unwrapped). Adds a lit test.
>From 7097f74fb236babbca1af536a160ce73ae0f2191 Mon Sep 17 00:00:00 2001
From: Kazuaki Matsumura <kmatsumura at nvidia.com>
Date: Wed, 24 Jun 2026 22:45:29 -0700
Subject: [PATCH 1/2] [mlir][acc] Fix mightHaveTerminator() assertion lowering
atomic capture under if clause
If-clause lowering builds a host-fallback path. While specializing it, the
acc.terminator erase pattern can remove a single-block region implicit
terminator before the orphan atomic-capture and region-unwrap patterns inline
that region via eraseOp(block->getTerminator()), tripping the
mightHaveTerminator() assertion. Guard the erase with mightHaveTerminator().
Adds an acc-if-clause-lowering regression test.
---
.../Transforms/ACCSpecializePatterns.h | 6 ++--
.../Transforms/ACCSpecializeForHost.cpp | 6 ++--
.../OpenACC/acc-if-clause-lowering.mlir | 34 ++++++++++++++++++-
3 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h b/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
index cde85fe839bf5..b4ae42b03d22c 100644
--- a/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
+++ b/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
@@ -97,8 +97,10 @@ class ACCRegionUnwrapConversion : public OpRewritePattern<OpTy> {
PatternRewriter &rewriter) const override {
assert(op.getRegion().hasOneBlock() && "expected one block");
Block *block = &op.getRegion().front();
- // Erase the terminator (acc.yield or acc.terminator) before unwrapping
- rewriter.eraseOp(block->getTerminator());
+ // The terminator may already be erased by the acc.terminator erase pattern
+ // during host fallback; only remove it if still present.
+ if (block->mightHaveTerminator())
+ rewriter.eraseOp(block->getTerminator());
rewriter.inlineBlockBefore(block, op);
rewriter.eraseOp(op);
return success();
diff --git a/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp b/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
index 633538069c268..26f10f8148fd7 100644
--- a/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
+++ b/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
@@ -256,8 +256,10 @@ class ACCOrphanAtomicCaptureOpConversion
assert(captureOp.getRegion().hasOneBlock() && "expected one block");
Block *block = &captureOp.getRegion().front();
- // Remove the terminator before inlining
- rewriter.eraseOp(block->getTerminator());
+ // The implicit acc.terminator may already be erased by the acc.terminator
+ // erase pattern during host fallback; only remove it if still present.
+ if (block->mightHaveTerminator())
+ rewriter.eraseOp(block->getTerminator());
rewriter.inlineBlockBefore(block, captureOp);
rewriter.eraseOp(captureOp);
return success();
diff --git a/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir b/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
index 4c88df432b6c7..342d67de77f3e 100644
--- a/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
@@ -373,4 +373,36 @@ func.func @test_acc_private(%arg0: memref<i32>, %cond: i1) {
acc.yield
}
return
-}
\ No newline at end of file
+}
+
+// -----
+
+// Regression: acc.parallel with an if clause whose body holds an
+// acc.atomic.capture. Host fallback inlines the capture; lowering must not
+// assert when the capture region terminator is erased before the capture op.
+// CHECK-LABEL: func.func @test_parallel_if_atomic_capture
+func.func @test_parallel_if_atomic_capture(%x: memref<i32>, %v: memref<i32>, %cond: i1) {
+ %c1_i32 = arith.constant 1 : i32
+ // CHECK-NOT: acc.parallel if
+ // CHECK: scf.if %{{.*}} {
+ // CHECK: acc.parallel {
+ // CHECK: acc.atomic.capture {
+ // CHECK: } else {
+ // CHECK-NOT: acc.atomic.capture
+ // CHECK: memref.load
+ // CHECK: arith.addi
+ // CHECK: memref.store
+ // CHECK: }
+ acc.parallel if(%cond) {
+ acc.atomic.capture {
+ acc.atomic.update %x : memref<i32> {
+ ^bb0(%arg: i32):
+ %r = arith.addi %arg, %c1_i32 : i32
+ acc.yield %r : i32
+ }
+ acc.atomic.read %v = %x : memref<i32>, memref<i32>, i32
+ }
+ acc.yield
+ }
+ return
+}
>From 608502e0173570f4a368e0c571e4ca2029e81f82 Mon Sep 17 00:00:00 2001
From: Kazuaki Matsumura <kmatsumura at nvidia.com>
Date: Wed, 24 Jun 2026 23:20:44 -0700
Subject: [PATCH 2/2] [mlir][acc] Restrict acc.terminator erase to stray
terminators
Address the root cause instead of guarding getTerminator(): the host-fallback
specialization registered a blanket acc.terminator erase pattern that matched
the implicit terminator of any still-present ACC region op, including
acc.atomic.capture. When it fired before the capture/region-unwrap patterns it
left the op without its required terminator, so the later getTerminator() call
tripped the mightHaveTerminator() assertion. Restrict the erase to terminators
no longer owned by an ACC region op; owning ops erase their own terminator when
unwrapped. Revert the defensive guards now that the invariant holds again.
---
.../Transforms/ACCSpecializePatterns.h | 6 ++--
.../Transforms/ACCSpecializeForHost.cpp | 32 +++++++++++++++----
.../OpenACC/acc-if-clause-lowering.mlir | 6 ++--
3 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h b/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
index b4ae42b03d22c..cde85fe839bf5 100644
--- a/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
+++ b/mlir/include/mlir/Dialect/OpenACC/Transforms/ACCSpecializePatterns.h
@@ -97,10 +97,8 @@ class ACCRegionUnwrapConversion : public OpRewritePattern<OpTy> {
PatternRewriter &rewriter) const override {
assert(op.getRegion().hasOneBlock() && "expected one block");
Block *block = &op.getRegion().front();
- // The terminator may already be erased by the acc.terminator erase pattern
- // during host fallback; only remove it if still present.
- if (block->mightHaveTerminator())
- rewriter.eraseOp(block->getTerminator());
+ // Erase the terminator (acc.yield or acc.terminator) before unwrapping
+ rewriter.eraseOp(block->getTerminator());
rewriter.inlineBlockBefore(block, op);
rewriter.eraseOp(op);
return success();
diff --git a/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp b/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
index 26f10f8148fd7..2dfd082388873 100644
--- a/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
+++ b/mlir/lib/Dialect/OpenACC/Transforms/ACCSpecializeForHost.cpp
@@ -256,16 +256,32 @@ class ACCOrphanAtomicCaptureOpConversion
assert(captureOp.getRegion().hasOneBlock() && "expected one block");
Block *block = &captureOp.getRegion().front();
- // The implicit acc.terminator may already be erased by the acc.terminator
- // erase pattern during host fallback; only remove it if still present.
- if (block->mightHaveTerminator())
- rewriter.eraseOp(block->getTerminator());
+ // Remove the terminator before inlining
+ rewriter.eraseOp(block->getTerminator());
rewriter.inlineBlockBefore(block, captureOp);
rewriter.eraseOp(captureOp);
return success();
}
};
+// Erase a stray acc.terminator only once it is no longer owned by a live ACC
+// region op; owning ops erase their own terminator when unwrapped.
+class ACCOrphanTerminatorEraseConversion
+ : public OpRewritePattern<acc::TerminatorOp> {
+ using OpRewritePattern<acc::TerminatorOp>::OpRewritePattern;
+
+public:
+ LogicalResult matchAndRewrite(acc::TerminatorOp op,
+ PatternRewriter &rewriter) const override {
+ if (Operation *parent = op->getParentOp())
+ if (parent->getDialect() ==
+ op->getContext()->getLoadedDialect<acc::OpenACCDialect>())
+ return failure();
+ rewriter.eraseOp(op);
+ return success();
+ }
+};
+
// Convert orphan acc.loop to scf.for or scf.execute_region.
// Only matches if NOT inside an ACC compute construct.
class ACCOrphanLoopOpConversion : public OpRewritePattern<acc::LoopOp> {
@@ -463,8 +479,12 @@ void mlir::acc::populateACCHostFallbackPatterns(RewritePatternSet &patterns,
// Runtime operations - erase them
patterns.insert<
ACCOpEraseConversion<acc::InitOp>, ACCOpEraseConversion<acc::ShutdownOp>,
- ACCOpEraseConversion<acc::SetOp>, ACCOpEraseConversion<acc::WaitOp>,
- ACCOpEraseConversion<acc::TerminatorOp>>(context);
+ ACCOpEraseConversion<acc::SetOp>, ACCOpEraseConversion<acc::WaitOp>>(
+ context);
+
+ // acc.terminator - erase only stray terminators no longer owned by an ACC
+ // region op; region-bearing ops erase their own terminator when unwrapped.
+ patterns.insert<ACCOrphanTerminatorEraseConversion>(context);
// Compute constructs - unwrap their regions
patterns.insert<ACCRegionUnwrapConversion<acc::ParallelOp>,
diff --git a/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir b/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
index 342d67de77f3e..af4cc72a42645 100644
--- a/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
+++ b/mlir/test/Dialect/OpenACC/acc-if-clause-lowering.mlir
@@ -377,9 +377,9 @@ func.func @test_acc_private(%arg0: memref<i32>, %cond: i1) {
// -----
-// Regression: acc.parallel with an if clause whose body holds an
-// acc.atomic.capture. Host fallback inlines the capture; lowering must not
-// assert when the capture region terminator is erased before the capture op.
+// Test that an acc.parallel with an if clause whose body holds an
+// acc.atomic.capture lowers cleanly: the device path keeps the capture and the
+// host fallback inlines it.
// CHECK-LABEL: func.func @test_parallel_if_atomic_capture
func.func @test_parallel_if_atomic_capture(%x: memref<i32>, %v: memref<i32>, %cond: i1) {
%c1_i32 = arith.constant 1 : i32
More information about the Mlir-commits
mailing list