Skip to content

Commit 8ec2112

Browse files
tblahtblah
authored
[OMPIRBuilder] re-land cancel barriers patch llvm#164586 (llvm#169931)
A barrier will pause execution until all threads reach it. If some go to a different barrier then we deadlock. This manifests in that the finalization callback must only be run once. Fix by ensuring we always go through the same finalization block whether the thread in cancelled or not and no matter which cancellation point causes the cancellation. The old callback only affected PARALLEL, so it has been moved into the code generating PARALLEL. For this reason, we don't need similar changes for other cancellable constructs. We need to create the barrier on the shared exit from the outlined function instead of only on the cancelled branch to make sure that threads exiting normally (without cancellation) meet the same barriers as those which were cancelled. For example, previously we might have generated code like ``` ... %ret = call i32 @__kmpc_cancel(...) %cond = icmp eq i32 %ret, 0 br i1 %cond, label %continue, label %cancel continue: // do the rest of the callback, eventually branching to %fini br label %fini cancel: // Populated by the callback: // unsafe: if any thread makes it to the end without being cancelled // it won't reach this barrier and then the program will deadlock %unused = call i32 @__kmpc_cancel_barrier(...) br label %fini fini: // run destructors etc ret ``` In the new version the barrier is moved into fini. I generate it *after* the destructors because the standard describes the barrier as occurring after the end of the parallel region. ``` ... %ret = call i32 @__kmpc_cancel(...) %cond = icmp eq i32 %ret, 0 br i1 %cond, label %continue, label %cancel continue: // do the rest of the callback, eventually branching to %fini br label %fini cancel: br label %fini fini: // run destructors etc // safe so long as every exit from the function happens via this block: %unused = call i32 @__kmpc_cancel_barrier(...) ret ``` To achieve this, the barrier is now generated alongside the finalization code instead of in the callback. This is the reason for the changes to the unit test. I'm unsure if I should keep the incorrect barrier generation callback only on the cancellation branch in clang with the OMPIRBuilder backend because that would match clang's ordinary codegen. Right now I have opted to remove it entirely because it is a deadlock waiting to happen. --- This re-lands llvm#164586 with a small fix for a failing buildbot running address sanitizer on clang lit tests. In the previous version of the patch I added an insertion point guard "just to be safe" and never removed it. There isn't insertion point guarding on the other route out of this function and we do not preserve the insertion point around getFiniBB either so it is not needed here. The problem flagged by the sanitizers was because the saved insertion point pointed to an instruction which was then removed inside the FiniCB for some clang codegen functions. The instruction was freed when it was removed. Then accessing it to restore the insertion point was a use after free bug.
1 parent 34c44f2 commit 8ec2112

23 files changed

+369
-262
lines changed

‎clang/test/OpenMP/cancel_codegen.cpp‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -774,8 +774,6 @@ for (int i = 0; i < argc; ++i) {
774774
// CHECK3-NEXT: call void @__kmpc_barrier(ptr @[[GLOB2:[0-9]+]], i32 [[OMP_GLOBAL_THREAD_NUM12]])
775775
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_AFTER:%.*]]
776776
// CHECK3: omp_section_loop.after:
777-
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_AFTERSECTIONS_FINI:%.*]]
778-
// CHECK3: omp_section_loop.aftersections.fini:
779777
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_PREHEADER13:%.*]]
780778
// CHECK3: omp_section_loop.preheader13:
781779
// CHECK3-NEXT: store i32 0, ptr [[P_LOWERBOUND29]], align 4
@@ -811,16 +809,16 @@ for (int i = 0; i < argc; ++i) {
811809
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_BODY_CASE23_SECTION_AFTER:%.*]]
812810
// CHECK3: omp_section_loop.body.case23.section.after:
813811
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_BODY16_SECTIONS_AFTER]]
814-
// CHECK3: omp_section_loop.body.case25:
812+
// CHECK3: omp_section_loop.body.case26:
815813
// CHECK3-NEXT: [[OMP_GLOBAL_THREAD_NUM27:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB1]])
816814
// CHECK3-NEXT: [[TMP18:%.*]] = call i32 @__kmpc_cancel(ptr @[[GLOB1]], i32 [[OMP_GLOBAL_THREAD_NUM27]], i32 3)
817815
// CHECK3-NEXT: [[TMP19:%.*]] = icmp eq i32 [[TMP18]], 0
818816
// CHECK3-NEXT: br i1 [[TMP19]], label [[OMP_SECTION_LOOP_BODY_CASE25_SPLIT:%.*]], label [[OMP_SECTION_LOOP_BODY_CASE25_CNCL:%.*]]
819-
// CHECK3: omp_section_loop.body.case25.split:
817+
// CHECK3: omp_section_loop.body.case26.split:
820818
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_BODY_CASE25_SECTION_AFTER26:%.*]]
821-
// CHECK3: omp_section_loop.body.case25.section.after26:
819+
// CHECK3: omp_section_loop.body.case26.section.after27:
822820
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_BODY_CASE25_SECTION_AFTER:%.*]]
823-
// CHECK3: omp_section_loop.body.case25.section.after:
821+
// CHECK3: omp_section_loop.body.case26.section.after:
824822
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_BODY16_SECTIONS_AFTER]]
825823
// CHECK3: omp_section_loop.body16.sections.after:
826824
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_INC17]]
@@ -833,8 +831,6 @@ for (int i = 0; i < argc; ++i) {
833831
// CHECK3-NEXT: call void @__kmpc_barrier(ptr @[[GLOB2]], i32 [[OMP_GLOBAL_THREAD_NUM33]])
834832
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_AFTER19:%.*]]
835833
// CHECK3: omp_section_loop.after19:
836-
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_AFTER19SECTIONS_FINI:%.*]]
837-
// CHECK3: omp_section_loop.after19sections.fini:
838834
// CHECK3-NEXT: [[TMP20:%.*]] = load i32, ptr [[ARGC_ADDR]], align 4
839835
// CHECK3-NEXT: store i32 [[TMP20]], ptr [[DOTCAPTURE_EXPR_]], align 4
840836
// CHECK3-NEXT: [[TMP21:%.*]] = load i32, ptr [[DOTCAPTURE_EXPR_]], align 4
@@ -894,8 +890,8 @@ for (int i = 0; i < argc; ++i) {
894890
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_EXIT]]
895891
// CHECK3: omp_section_loop.body.case23.cncl:
896892
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_EXIT18]]
897-
// CHECK3: omp_section_loop.body.case25.cncl:
898-
// CHECK3-NEXT: br label [[OMP_SECTION_LOOP_EXIT18]]
893+
// CHECK3: omp_section_loop.body.case26.cncl:
894+
// CHECK3-NEXT: br label [[OMP_REGION_FINALIZE:.*]]
899895
// CHECK3: .cancel.continue:
900896
// CHECK3-NEXT: br label [[OMP_IF_END:%.*]]
901897
// CHECK3: omp_if.else:
@@ -967,8 +963,10 @@ for (int i = 0; i < argc; ++i) {
967963
// CHECK3-NEXT: [[TMP8:%.*]] = call i32 @__kmpc_cancel_barrier(ptr @[[GLOB3:[0-9]+]], i32 [[OMP_GLOBAL_THREAD_NUM4]])
968964
// CHECK3-NEXT: [[TMP9:%.*]] = icmp eq i32 [[TMP8]], 0
969965
// CHECK3-NEXT: br i1 [[TMP9]], label [[DOTCONT:%.*]], label [[DOTCNCL5:%.*]]
970-
// CHECK3: .cncl5:
971-
// CHECK3-NEXT: br label [[OMP_PAR_OUTLINED_EXIT_EXITSTUB:%.*]]
966+
// CHECK3: .cncl4:
967+
// CHECK3-NEXT: br label [[FINI:%.*]]
968+
// CHECK3: .fini
969+
// CHECK3-NEXT: br label %[[EXIT_STUB:omp.par.exit.exitStub]]
972970
// CHECK3: .cont:
973971
// CHECK3-NEXT: [[TMP10:%.*]] = load i32, ptr [[LOADGEP_ARGC_ADDR]], align 4
974972
// CHECK3-NEXT: [[TMP11:%.*]] = load ptr, ptr [[LOADGEP_ARGV_ADDR]], align 8
@@ -984,16 +982,14 @@ for (int i = 0; i < argc; ++i) {
984982
// CHECK3: omp.par.region.parallel.after:
985983
// CHECK3-NEXT: br label [[OMP_PAR_PRE_FINALIZE:%.*]]
986984
// CHECK3: omp.par.pre_finalize:
987-
// CHECK3-NEXT: br label [[OMP_PAR_OUTLINED_EXIT_EXITSTUB]]
985+
// CHECK3-NEXT: br label [[FINI]]
988986
// CHECK3: 14:
989987
// CHECK3-NEXT: [[OMP_GLOBAL_THREAD_NUM1:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB1]])
990988
// CHECK3-NEXT: [[TMP15:%.*]] = call i32 @__kmpc_cancel(ptr @[[GLOB1]], i32 [[OMP_GLOBAL_THREAD_NUM1]], i32 1)
991989
// CHECK3-NEXT: [[TMP16:%.*]] = icmp eq i32 [[TMP15]], 0
992990
// CHECK3-NEXT: br i1 [[TMP16]], label [[DOTSPLIT:%.*]], label [[DOTCNCL:%.*]]
993991
// CHECK3: .cncl:
994-
// CHECK3-NEXT: [[OMP_GLOBAL_THREAD_NUM2:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB1]])
995-
// CHECK3-NEXT: [[TMP17:%.*]] = call i32 @__kmpc_cancel_barrier(ptr @[[GLOB2]], i32 [[OMP_GLOBAL_THREAD_NUM2]])
996-
// CHECK3-NEXT: br label [[OMP_PAR_OUTLINED_EXIT_EXITSTUB]]
992+
// CHECK3-NEXT: br label [[FINI]]
997993
// CHECK3: .split:
998994
// CHECK3-NEXT: br label [[TMP4]]
999995
// CHECK3: omp.par.exit.exitStub:
@@ -1089,7 +1085,7 @@ for (int i = 0; i < argc; ++i) {
10891085
// CHECK3: .omp.sections.case.split:
10901086
// CHECK3-NEXT: br label [[DOTOMP_SECTIONS_EXIT]]
10911087
// CHECK3: .omp.sections.case.cncl:
1092-
// CHECK3-NEXT: br label [[CANCEL_CONT:%.*]]
1088+
// CHECK3-NEXT: br label [[FINI:%.*]]
10931089
// CHECK3: .omp.sections.exit:
10941090
// CHECK3-NEXT: br label [[OMP_INNER_FOR_INC:%.*]]
10951091
// CHECK3: omp.inner.for.inc:
@@ -1100,7 +1096,7 @@ for (int i = 0; i < argc; ++i) {
11001096
// CHECK3: omp.inner.for.end:
11011097
// CHECK3-NEXT: [[OMP_GLOBAL_THREAD_NUM3:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB19:[0-9]+]])
11021098
// CHECK3-NEXT: call void @__kmpc_for_static_fini(ptr @[[GLOB15]], i32 [[OMP_GLOBAL_THREAD_NUM3]])
1103-
// CHECK3-NEXT: br label [[CANCEL_CONT]]
1099+
// CHECK3-NEXT: br label [[CANCEL_CONT:.*]]
11041100
// CHECK3: cancel.cont:
11051101
// CHECK3-NEXT: ret void
11061102
// CHECK3: cancel.exit:
@@ -1153,6 +1149,8 @@ for (int i = 0; i < argc; ++i) {
11531149
// CHECK3: .omp.sections.case.split:
11541150
// CHECK3-NEXT: br label [[DOTOMP_SECTIONS_EXIT]]
11551151
// CHECK3: .omp.sections.case.cncl:
1152+
// CHECK3-NEXT: br label [[DOTFINI:.%*]]
1153+
// CHECK3: .fini:
11561154
// CHECK3-NEXT: br label [[CANCEL_CONT:%.*]]
11571155
// CHECK3: .omp.sections.case2:
11581156
// CHECK3-NEXT: [[OMP_GLOBAL_THREAD_NUM3:%.*]] = call i32 @__kmpc_global_thread_num(ptr @[[GLOB1]])
@@ -1162,9 +1160,11 @@ for (int i = 0; i < argc; ++i) {
11621160
// CHECK3: .omp.sections.case2.split:
11631161
// CHECK3-NEXT: br label [[DOTOMP_SECTIONS_CASE2_SECTION_AFTER:%.*]]
11641162
// CHECK3: .omp.sections.case2.section.after:
1165-
// CHECK3-NEXT: br label [[DOTOMP_SECTIONS_EXIT]]
1163+
// CHECK3-NEXT: br label [[OMP_REGION_FINALIZE]]
1164+
// CHECK3: omp_region.finalize:
1165+
// CHECK3-NEXT: br label [[OMP_SECTIONS_EXIT:.*]]
11661166
// CHECK3: .omp.sections.case2.cncl:
1167-
// CHECK3-NEXT: br label [[OMP_INNER_FOR_END]]
1167+
// CHECK3-NEXT: br label [[FINI:.*]]
11681168
// CHECK3: .omp.sections.exit:
11691169
// CHECK3-NEXT: br label [[OMP_INNER_FOR_INC:%.*]]
11701170
// CHECK3: omp.inner.for.inc:

‎clang/test/OpenMP/critical_codegen.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ int main() {
3535
// ALL-NEXT: store i8 2, ptr [[A_ADDR]]
3636
// IRBUILDER-NEXT: br label %[[AFTER:[^ ,]+]]
3737
// IRBUILDER: [[AFTER]]
38+
// IRBUILDER-NEXT: br label %[[OMP_REGION_FINALIZE:[^ ,]+]]
39+
// IRBUILDER: [[OMP_REGION_FINALIZE]]
3840
// ALL-NEXT: call {{.*}}void @__kmpc_end_critical(ptr [[DEFAULT_LOC]], i32 [[GTID]], ptr [[UNNAMED_LOCK]])
3941
#pragma omp critical
4042
a = 2;

‎clang/test/OpenMP/critical_codegen_attr.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ int main() {
3535
// ALL-NEXT: store i8 2, ptr [[A_ADDR]]
3636
// IRBUILDER-NEXT: br label %[[AFTER:[^ ,]+]]
3737
// IRBUILDER: [[AFTER]]
38+
// IRBUILDER-NEXT: br label %[[OMP_REGION_FINALIZE:[^ ,]+]]
39+
// IRBUILDER: [[OMP_REGION_FINALIZE]]
3840
// ALL-NEXT: call {{.*}}void @__kmpc_end_critical(ptr [[DEFAULT_LOC]], i32 [[GTID]], ptr [[UNNAMED_LOCK]])
3941
[[omp::directive(critical)]]
4042
a = 2;

0 commit comments

Comments
 (0)