[mlir][OpenMP] fix crash outlining infinite loop #129872
Conversation
Previously an extra block was created by splitting the previous exit block. This produced incorrect results when the outlined region statically never terminated because then there wouldn't be a valid exit block for the outlined region, this caused this newly added block to have an incoming edge from outside of the outlining region, which caused outlining to fail. So far as I can tell this extra block no longer serves any purpose. The comment says it is supposed to collate multiple control flow edges into one place, but the code as it is now does not achieve this. In fact, as can be seen from the changes to lit tests, this block was not actually outlined in the end. This is because there are actually two code extractors: one in the callback for creating a parallel op which is used to find what the input/output variables are (which does have this block added to it), and another one which actually does the outlining (which this block was not added to). Tested with the gfortran and fujitsu test suites.
tblah
requested review from
Meinersbur,
skatrak,
kiranchandramohan and
jdoerfert
llvmbot
added
mlir:llvm
mlir
flang
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-flang-openmp Author: Tom Eccles (tblah) ChangesPreviously an extra block was created by splitting the previous exit block. This produced incorrect results when the outlined region statically never terminated because then there wouldn't be a valid exit block for the outlined region, this caused this newly added block to have an incoming edge from outside of the outlining region, which caused outlining to fail. So far as I can tell this extra block no longer serves any purpose. The comment says it is supposed to collate multiple control flow edges into one place, but the code as it is now does not achieve this. In fact, as can be seen from the changes to lit tests, this block was not actually outlined in the end. This is because there are actually two code extractors: one in the callback for creating a parallel op which is used to find what the input/output variables are (which does have this block added to it), and another one which actually does the outlining (which this block was not added to). Tested with the gfortran and fujitsu test suites. Fixes #112884 Full diff: https://github.com/llvm/llvm-project/pull/129872.diff 8 Files Affected:
diff --git a/flang/test/Integration/OpenMP/parallel-private-reduction-worstcase.f90 b/flang/test/Integration/OpenMP/parallel-private-reduction-worstcase.f90
index 7e735b6499504..cf77c46346b7f 100644
--- a/flang/test/Integration/OpenMP/parallel-private-reduction-worstcase.f90
+++ b/flang/test/Integration/OpenMP/parallel-private-reduction-worstcase.f90
@@ -219,5 +219,5 @@ subroutine worst_case(a, b, c, d)
! [var extent was non-zero: malloc a private array]
! CHECK: br label %omp.private.init5
-! CHECK: omp.par.outlined.exit.exitStub: ; preds = %omp.region.cont52
+! CHECK: omp.par.exit.exitStub: ; preds = %omp.region.cont52
! CHECK-NEXT: ret void
diff --git a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
index e34e93442ff85..0295319ec1e9d 100644
--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -1602,14 +1602,6 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::createParallel(
SmallVector<BasicBlock *, 32> Blocks;
OI.collectBlocks(ParallelRegionBlockSet, Blocks);
- // Ensure a single exit node for the outlined region by creating one.
- // We might have multiple incoming edges to the exit now due to finalizations,
- // e.g., cancel calls that cause the control flow to leave the region.
- BasicBlock *PRegOutlinedExitBB = PRegExitBB;
- PRegExitBB = SplitBlock(PRegExitBB, &*PRegExitBB->getFirstInsertionPt());
- PRegOutlinedExitBB->setName("omp.par.outlined.exit");
- Blocks.push_back(PRegOutlinedExitBB);
-
CodeExtractorAnalysisCache CEAC(*OuterFn);
CodeExtractor Extractor(Blocks, /* DominatorTree */ nullptr,
/* AggregateArgs */ false,
diff --git a/mlir/test/Target/LLVMIR/openmp-llvm.mlir b/mlir/test/Target/LLVMIR/openmp-llvm.mlir
index f25ba4aa3c8dc..e4114d491fc85 100644
--- a/mlir/test/Target/LLVMIR/openmp-llvm.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-llvm.mlir
@@ -162,14 +162,12 @@ llvm.func @test_omp_parallel_if_1(%arg0: i32) -> () {
// CHECK: %[[I32_IF_COND_VAR_1:.*]] = sext i1 %[[IF_COND_VAR_1]] to i32
// CHECK: call void @__kmpc_fork_call_if(ptr @[[SI_VAR_IF_1]], i32 0, ptr @[[OMP_OUTLINED_FN_IF_1:.*]], i32 %[[I32_IF_COND_VAR_1]], ptr null)
// CHECK: br label %[[OUTLINED_EXIT_IF_1:.*]]
-// CHECK: [[OUTLINED_EXIT_IF_1]]:
-// CHECK: br label %[[RETURN_BLOCK_IF_1:.*]]
omp.parallel if(%1) {
omp.barrier
omp.terminator
}
-// CHECK: [[RETURN_BLOCK_IF_1]]:
+// CHECK: [[OUTLINED_EXIT_IF_1]]:
// CHECK: ret void
llvm.return
}
diff --git a/mlir/test/Target/LLVMIR/openmp-outline-infinite-loop.mlir b/mlir/test/Target/LLVMIR/openmp-outline-infinite-loop.mlir
new file mode 100644
index 0000000000000..faccfc678adfe
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/openmp-outline-infinite-loop.mlir
@@ -0,0 +1,44 @@
+// RUN: mlir-translate -mlir-to-llvmir %s | FileCheck %s
+
+// Test that trying to outline an infinite loop doesn't lead to an assertion
+// failure.
+
+llvm.func @parallel_infinite_loop() -> () {
+ omp.parallel {
+ llvm.br ^bb1
+ ^bb1:
+ llvm.br ^bb1
+ }
+ llvm.return
+}
+
+// CHECK-LABEL: define void @parallel_infinite_loop() {
+// CHECK: %[[VAL_2:.*]] = call i32 @__kmpc_global_thread_num(ptr @1)
+// CHECK: br label %[[VAL_3:.*]]
+// CHECK: omp_parallel:
+// CHECK: call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @1, i32 0, ptr @parallel_infinite_loop..omp_par)
+// CHECK: unreachable
+// CHECK: omp.region.cont: ; No predecessors!
+// CHECK: br label %[[VAL_4:.*]]
+// CHECK: omp.par.pre_finalize: ; preds = %[[VAL_5:.*]]
+// CHECK: br label %[[VAL_6:.*]]
+// CHECK: omp.par.exit: ; preds = %[[VAL_4]]
+// CHECK: ret void
+// CHECK: }
+
+// CHECK-LABEL: define internal void @parallel_infinite_loop..omp_par(
+// CHECK-SAME: ptr noalias %[[TID_ADDR:.*]], ptr noalias %[[ZERO_ADDR:.*]])
+// CHECK: omp.par.entry:
+// CHECK: %[[VAL_7:.*]] = alloca i32, align 4
+// CHECK: %[[VAL_8:.*]] = load i32, ptr %[[VAL_9:.*]], align 4
+// CHECK: store i32 %[[VAL_8]], ptr %[[VAL_7]], align 4
+// CHECK: %[[VAL_10:.*]] = load i32, ptr %[[VAL_7]], align 4
+// CHECK: br label %[[VAL_11:.*]]
+// CHECK: omp.region.after_alloca: ; preds = %[[VAL_12:.*]]
+// CHECK: br label %[[VAL_13:.*]]
+// CHECK: omp.par.region: ; preds = %[[VAL_11]]
+// CHECK: br label %[[VAL_14:.*]]
+// CHECK: omp.par.region1: ; preds = %[[VAL_13]]
+// CHECK: br label %[[VAL_15:.*]]
+// CHECK: omp.par.region2: ; preds = %[[VAL_15]], %[[VAL_14]]
+// CHECK: br label %[[VAL_15]]
diff --git a/mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir b/mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir
index d2e394b2cf6a8..887d2977e45cc 100644
--- a/mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-parallel-reduction-multiblock.mlir
@@ -40,9 +40,7 @@ llvm.func @missordered_blocks_(%arg0: !llvm.ptr {fir.bindc_name = "x"}, %arg1: !
// CHECK: store ptr %[[VAL_8:.*]], ptr %[[VAL_7]], align 8
// CHECK: call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @1, i32 1, ptr @missordered_blocks_..omp_par, ptr %[[VAL_0]])
// CHECK: br label %[[VAL_9:.*]]
-// CHECK: omp.par.outlined.exit: ; preds = %[[VAL_4]]
-// CHECK: br label %[[VAL_10:.*]]
-// CHECK: omp.par.exit.split: ; preds = %[[VAL_9]]
+// CHECK: omp.par.exit: ; preds = %[[VAL_4]]
// CHECK: ret void
// CHECK: [[PAR_ENTRY:omp.par.entry]]:
// CHECK: %[[VAL_11:.*]] = getelementptr { ptr, ptr }, ptr %[[VAL_12:.*]], i32 0, i32 0
@@ -117,5 +115,5 @@ llvm.func @missordered_blocks_(%arg0: !llvm.ptr {fir.bindc_name = "x"}, %arg1: !
// CHECK: br label %[[VAL_38]]
// CHECK: omp.reduction.neutral1: ; preds = %[[VAL_25]]
// CHECK: br label %[[VAL_30]]
-// CHECK: omp.par.outlined.exit.exitStub: ; preds = %[[VAL_53]]
+// CHECK: omp.par.exit.exitStub: ; preds = %[[VAL_53]]
// CHECK: ret void
diff --git a/mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir b/mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir
index d6ed3086969fb..b302b4b20edd5 100644
--- a/mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-reduction-array-sections.mlir
@@ -219,5 +219,5 @@ llvm.func @sectionsreduction_(%arg0: !llvm.ptr {fir.bindc_name = "x"}) attribute
// CHECK: omp_section_loop.inc: ; preds = %[[VAL_69]]
// CHECK: %[[VAL_31]] = add nuw i32 %[[VAL_30]], 1
// CHECK: br label %[[VAL_28]]
-// CHECK: omp.par.outlined.exit.exitStub: ; preds = %[[VAL_64]]
+// CHECK: omp.par.exit.exitStub: ; preds = %[[VAL_64]]
// CHECK: ret void
diff --git a/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir b/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
index 8d329bd8ff817..a714ca68a1e95 100644
--- a/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-reduction-init-arg.mlir
@@ -46,9 +46,7 @@ module {
// CHECK: store ptr %[[VAL_2]], ptr %[[VAL_8]], align 8
// CHECK: call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @1, i32 1, ptr @_QFPreduce..omp_par, ptr %[[VAL_0]])
// CHECK: br label %[[VAL_9:.*]]
-// CHECK: omp.par.outlined.exit: ; preds = %[[VAL_6]]
-// CHECK: br label %[[VAL_10:.*]]
-// CHECK: omp.par.exit.split: ; preds = %[[VAL_9]]
+// CHECK: omp.par.exit: ; preds = %[[VAL_6]]
// CHECK: ret void
// CHECK: [[PAR_ENTRY:omp.par.entry]]:
// CHECK: %[[VAL_11:.*]] = getelementptr { ptr, ptr }, ptr %[[VAL_12:.*]], i32 0, i32 0
@@ -99,7 +97,7 @@ module {
// CHECK: br label %[[VAL_38:.*]]
// CHECK: omp.par.pre_finalize: ; preds = %[[VAL_33]]
// CHECK: br label %[[VAL_39:.*]]
-// CHECK: omp.par.outlined.exit.exitStub: ; preds = %[[VAL_38]]
+// CHECK: omp.par.exit.exitStub: ; preds = %[[VAL_38]]
// CHECK: ret void
// CHECK: %[[VAL_40:.*]] = getelementptr inbounds [2 x ptr], ptr %[[VAL_41:.*]], i64 0, i64 0
// CHECK: %[[VAL_42:.*]] = load ptr, ptr %[[VAL_40]], align 8
diff --git a/mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir b/mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir
index de3b997feb674..19da6f8517fcd 100644
--- a/mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir
+++ b/mlir/test/Target/LLVMIR/openmp-reduction-sections.mlir
@@ -144,7 +144,7 @@ llvm.func @sections_(%arg0: !llvm.ptr {fir.bindc_name = "x"}) attributes {fir.in
// CHECK: omp_section_loop.inc: ; preds = %[[VAL_59]]
// CHECK: %[[VAL_35]] = add nuw i32 %[[VAL_34]], 1
// CHECK: br label %[[VAL_32]]
-// CHECK: omp.par.outlined.exit.exitStub: ; preds = %[[VAL_54]]
+// CHECK: omp.par.exit.exitStub: ; preds = %[[VAL_54]]
// CHECK: ret void
// CHECK: %[[VAL_70:.*]] = getelementptr inbounds [1 x ptr], ptr %[[VAL_71:.*]], i64 0, i64 0
// CHECK: %[[VAL_72:.*]] = load ptr, ptr %[[VAL_70]], align 8
|
|
The code was added in 70cac41 The comment mentions the additional edges might come through cancellations, but what test actually tests cancellation points? |
Thanks for taking a look. Now I have added clang lit tests (including for cancel) we can see that there are no changes beyond block names and removing a redundant block. The commit description you mentioned says it is trying to avoid returning values from the outlined function, which is not something which has been seen in any of the lit tests. Do you know of any clang test suites that are known to work for openmpirbuilder that I could try with? |
There is probably no cancellation support at the moment. I remember that you had a set of patches to enable support for that but never landed. |
Eg your patch talks about cancellation: https://reviews.llvm.org/D124823 The OpenMPIRBuilder patch for cancel was https://reviews.llvm.org/D71948 |
There was a problem hiding this comment.
After a hard time getting even #114419 approved, I don't think anybody is really interested in reviewing/fixing non-SESE regions. With cancellation points basically not supported, I guess it doesn't matter whether this PR breaks it even more.
OpenMP 4.0 support will require cancellation. What would we do at that point? Is your patch strictly required for cancellation? |
Thanks Michael. I don't expect this to block cancellation support. As I understand it, the problem this was solving was when there were multiple different return points from the outlined function. There seemed to be cases of this in the clang lit tests and those also show no difference other than block naming and removing a redundant block. I expect the bug this extra block was working around has now been fixed elsewhere. And anyway, like I said, this block isn't actually included in the outlined region because there are two different code extractor instances and this block is only added to one of them. Therefore I think wouldn't have stopped multiple return points anyway. |
|
@tblah I don't think it is blocking cancellation, we just don't know what is going on. The Footnotes
|
Previously an extra block was created by splitting the previous exit block. This produced incorrect results when the outlined region statically never terminated because then there wouldn't be a valid exit block for the outlined region, this caused this newly added block to have an incoming edge from outside of the outlining region, which caused outlining to fail. So far as I can tell this extra block no longer serves any purpose. The comment says it is supposed to collate multiple control flow edges into one place, but the code as it is now does not achieve this. In fact, as can be seen from the changes to lit tests, this block was not actually outlined in the end. This is because there are actually two code extractors: one in the callback for creating a parallel op which is used to find what the input/output variables are (which does have this block added to it), and another one which actually does the outlining (which this block was not added to). Tested with the gfortran and fujitsu test suites. Fixes llvm#112884
Previously an extra block was created by splitting the previous exit block. This produced incorrect results when the outlined region statically never terminated because then there wouldn't be a valid exit block for the outlined region, this caused this newly added block to have an incoming edge from outside of the outlining region, which caused outlining to fail.
So far as I can tell this extra block no longer serves any purpose. The comment says it is supposed to collate multiple control flow edges into one place, but the code as it is now does not achieve this. In fact, as can be seen from the changes to lit tests, this block was not actually outlined in the end. This is because there are actually two code extractors: one in the callback for creating a parallel op which is used to find what the input/output variables are (which does have this block added to it), and another one which actually does the outlining (which this block was not added to).
Tested with the gfortran and fujitsu test suites.
Fixes #112884