Skip to content

Commit 748e7af

Browse files
KrxGuKrish Gupta
KrxGu
and
Krish Gupta
authored
[flang][OpenMP] Fix firstprivate not working with lastprivate in DO SIMD (#170163)
This fixes a bug where firstprivate was ignored when the same variable had both firstprivate and lastprivate clauses in a do simd construct. What was broken: ``` integer :: a a = 10 !$omp do simd firstprivate(a) lastprivate(a) do i = 1, 1 print *, a ! Should print 10, but printed garbage/0 a = 20 end do !$omp end do simd print *, a ! Correctly prints 20 ``` Inside the loop, [a] wasn't being initialized from the firstprivate clause—it just had whatever uninitialized value was there. The fix: In genCompositeDoSimd(), we were using simdItemDSP to handle privatization for the whole loop nest. This only looked at SIMD clauses and missed the firstprivate from the DO part. Changed it to use wsloopItemDSP instead, which handles both DO clauses (firstprivate, lastprivate) correctly. One line change in OpenMP.cpp Tests added: Lowering test to check MLIR generation Runtime test to verify the actual values are correct <img width="740" height="440" alt="image" src="https://github.com/user-attachments/assets/fa911ea8-2024-4edf-b710-52c10659742e" /> Fixes #168306 --------- Co-authored-by: Krish Gupta <[email protected]>
1 parent db59def commit 748e7af

File tree

5 files changed

+151
-23
lines changed

5 files changed

+151
-23
lines changed

‎flang/lib/Lower/OpenMP/OpenMP.cpp‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3314,17 +3314,12 @@ static mlir::omp::WsloopOp genCompositeDoSimd(
33143314
genSimdClauses(converter, semaCtx, simdItem->clauses, loc, simdClauseOps,
33153315
simdReductionSyms);
33163316

3317-
DataSharingProcessor wsloopItemDSP(
3318-
converter, semaCtx, doItem->clauses, eval,
3319-
/*shouldCollectPreDeterminedSymbols=*/false,
3320-
/*useDelayedPrivatization=*/true, symTable);
3317+
DataSharingProcessor wsloopItemDSP(converter, semaCtx, doItem->clauses, eval,
3318+
/*shouldCollectPreDeterminedSymbols=*/true,
3319+
/*useDelayedPrivatization=*/true,
3320+
symTable);
33213321
wsloopItemDSP.processStep1(&wsloopClauseOps);
33223322

3323-
DataSharingProcessor simdItemDSP(converter, semaCtx, simdItem->clauses, eval,
3324-
/*shouldCollectPreDeterminedSymbols=*/true,
3325-
/*useDelayedPrivatization=*/true, symTable);
3326-
simdItemDSP.processStep1(&simdClauseOps, simdItem->id);
3327-
33283323
// Pass the innermost leaf construct's clauses because that's where COLLAPSE
33293324
// is placed by construct decomposition.
33303325
mlir::omp::LoopNestOperands loopNestClauseOps;
@@ -3343,8 +3338,9 @@ static mlir::omp::WsloopOp genCompositeDoSimd(
33433338
wsloopOp.setComposite(/*val=*/true);
33443339

33453340
EntryBlockArgs simdArgs;
3346-
simdArgs.priv.syms = simdItemDSP.getDelayedPrivSymbols();
3347-
simdArgs.priv.vars = simdClauseOps.privateVars;
3341+
// For composite 'do simd', privatization is handled by the wsloop.
3342+
// The simd does not create separate private storage for variables already
3343+
// privatized by the worksharing construct.
33483344
simdArgs.reduction.syms = simdReductionSyms;
33493345
simdArgs.reduction.vars = simdClauseOps.reductionVars;
33503346
auto simdOp =
@@ -3354,7 +3350,7 @@ static mlir::omp::WsloopOp genCompositeDoSimd(
33543350
genLoopNestOp(converter, symTable, semaCtx, eval, loc, queue, simdItem,
33553351
loopNestClauseOps, iv,
33563352
{{wsloopOp, wsloopArgs}, {simdOp, simdArgs}},
3357-
llvm::omp::Directive::OMPD_do_simd, simdItemDSP);
3353+
llvm::omp::Directive::OMPD_do_simd, wsloopItemDSP);
33583354
return wsloopOp;
33593355
}
33603356

‎flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
! Test runtime behavior of DO SIMD with firstprivate and lastprivate on same variable
2+
! This is the reproducer from issue #168306
3+
4+
! REQUIRES: openmp-runtime
5+
6+
! RUN: %flang_fc1 -fopenmp -emit-llvm %s -o - | FileCheck %s --check-prefix=LLVM
7+
! RUN: %flang -fopenmp %s -o %t && %t | FileCheck %s
8+
9+
! LLVM-LABEL: define {{.*}} @_QQmain
10+
program main
11+
integer :: a
12+
integer :: i
13+
14+
a = 10
15+
!$omp do simd lastprivate(a) firstprivate(a)
16+
do i = 1, 1
17+
! Inside loop: a should be 10 (from firstprivate initialization)
18+
! CHECK: main1 : a = 10
19+
print *, "main1 : a = ", a
20+
a = 20
21+
end do
22+
!$omp end do simd
23+
! After loop: a should be 20 (from lastprivate copy-out)
24+
! CHECK: main2 : a = 20
25+
print *, "main2 : a = ", a
26+
27+
call sub
28+
! CHECK: pass
29+
print *, 'pass'
30+
end program main
31+
32+
subroutine sub
33+
integer :: a
34+
integer :: i
35+
36+
a = 10
37+
!$omp do simd lastprivate(a) firstprivate(a)
38+
do i = 1, 1
39+
! Inside loop: a should be 10 (from firstprivate initialization)
40+
! CHECK: sub1 : a = 10
41+
print *, "sub1 : a = ", a
42+
a = 20
43+
end do
44+
!$omp end do simd
45+
! After loop: a should be 20 (from lastprivate copy-out)
46+
! CHECK: sub2 : a = 20
47+
print *, "sub2 : a = ", a
48+
end subroutine sub

‎flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
! Test for DO SIMD with the same variable in both firstprivate and lastprivate clauses
2+
! This tests the fix for issue #168306
3+
4+
! RUN: %flang_fc1 -fopenmp -mmlir --enable-delayed-privatization-staging=true -emit-hlfir %s -o - | FileCheck %s
5+
6+
! Test case 1: Basic test with firstprivate + lastprivate on same variable
7+
! CHECK-LABEL: func.func @_QPdo_simd_first_last_same_var
8+
subroutine do_simd_first_last_same_var()
9+
integer :: a
10+
integer :: i
11+
a = 10
12+
13+
! CHECK: omp.wsloop
14+
! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]], @{{.*}}private{{.*}} %{{.*}} -> %[[PRIV_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
15+
! CHECK-NEXT: omp.simd
16+
! CHECK-NOT: private
17+
! CHECK-NEXT: omp.loop_nest (%[[IV:.*]]) : i32
18+
!$omp do simd firstprivate(a) lastprivate(a)
19+
do i = 1, 1
20+
! CHECK: %[[FIRSTPRIV_A_DECL:.*]]:2 = hlfir.declare %[[FIRSTPRIV_A]]
21+
! CHECK: %[[PRIV_I_DECL:.*]]:2 = hlfir.declare %[[PRIV_I]]
22+
! The private copy should be initialized from firstprivate (value 10)
23+
! and then modified to 20
24+
a = 20
25+
end do
26+
!$omp end do simd
27+
! After the loop, 'a' should be 20 due to lastprivate
28+
end subroutine do_simd_first_last_same_var
29+
30+
! Test case 2: Test with lastprivate and firstprivate in reverse order
31+
! CHECK-LABEL: func.func @_QPdo_simd_last_first_reverse
32+
subroutine do_simd_last_first_reverse()
33+
integer :: a
34+
integer :: i
35+
a = 10
36+
37+
! CHECK: omp.wsloop
38+
! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]], @{{.*}}private{{.*}} %{{.*}} -> %[[PRIV_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
39+
! CHECK-NEXT: omp.simd
40+
! CHECK-NOT: private
41+
!$omp do simd lastprivate(a) firstprivate(a)
42+
do i = 1, 1
43+
a = 20
44+
end do
45+
!$omp end do simd
46+
end subroutine do_simd_last_first_reverse
47+
48+
! Test case 3: Multiple variables with mixed privatization
49+
! CHECK-LABEL: func.func @_QPdo_simd_multiple_vars
50+
subroutine do_simd_multiple_vars()
51+
integer :: a, b, c
52+
integer :: i
53+
a = 10
54+
b = 20
55+
c = 30
56+
57+
! CHECK: omp.wsloop
58+
! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %{{.*}}, @{{.*}}firstprivate{{.*}} %{{.*}} -> %{{.*}}, @{{.*}}private{{.*}} %{{.*}} -> %{{.*}} : !fir.ref<i32>, !fir.ref<i32>, !fir.ref<i32>)
59+
! CHECK-NEXT: omp.simd
60+
! CHECK-NOT: private
61+
!$omp do simd firstprivate(a, b) lastprivate(a) private(c)
62+
do i = 1, 5
63+
a = a + 1
64+
b = b + 1
65+
c = i
66+
end do
67+
!$omp end do simd
68+
end subroutine do_simd_multiple_vars
69+
70+
! Test case 4: Reproducer from issue #168306
71+
! CHECK-LABEL: func.func @_QPissue_168306_reproducer
72+
subroutine issue_168306_reproducer()
73+
integer :: a
74+
integer :: i
75+
a = 10
76+
77+
! CHECK: omp.wsloop
78+
! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]], @{{.*}}private{{.*}} %{{.*}} -> %[[PRIV_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
79+
! CHECK-NEXT: omp.simd
80+
! CHECK-NOT: private
81+
!$omp do simd lastprivate(a) firstprivate(a)
82+
do i = 1, 1
83+
! Inside the loop, 'a' should start at 10 (from firstprivate)
84+
! This is the key behavior that was broken
85+
a = 20
86+
end do
87+
!$omp end do simd
88+
! After the loop, 'a' should be 20 (from lastprivate)
89+
end subroutine issue_168306_reproducer

‎flang/test/Lower/OpenMP/order-clause.f90‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,15 +36,15 @@ end subroutine do_order
3636

3737
!CHECK-LABEL: func.func @_QPdo_simd_order() {
3838
subroutine do_simd_order
39-
!CHECK: omp.wsloop order(reproducible:concurrent) {
39+
!CHECK: omp.wsloop order(reproducible:concurrent)
4040
!$omp do simd order(concurrent)
4141
do i = 1, 10
4242
end do
43-
!CHECK: omp.wsloop order(reproducible:concurrent) {
43+
!CHECK: omp.wsloop order(reproducible:concurrent)
4444
!$omp do simd order(reproducible:concurrent)
4545
do i = 1, 10
4646
end do
47-
!CHECK: omp.wsloop order(unconstrained:concurrent) {
47+
!CHECK: omp.wsloop order(unconstrained:concurrent)
4848
!$omp do simd order(unconstrained:concurrent)
4949
do i = 1, 10
5050
end do
@@ -53,7 +53,7 @@ end subroutine do_simd_order
5353
!CHECK-LABEL: func.func @_QPdo_simd_order_parallel() {
5454
subroutine do_simd_order_parallel
5555
!CHECK: omp.parallel {
56-
!CHECK: omp.wsloop order(reproducible:concurrent) {
56+
!CHECK: omp.wsloop order(reproducible:concurrent)
5757
!$omp parallel do simd order(reproducible:concurrent)
5858
do i = 1, 10
5959
end do

‎flang/test/Lower/OpenMP/wsloop-simd.f90‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,13 @@ end subroutine do_simd_reduction
7171
subroutine do_simd_private()
7272
integer, allocatable :: tmp
7373
! CHECK: omp.wsloop
74+
! CHECK-SAME: private(@[[PRIV_IVAR_SYM:.*]] %{{.*}} -> %[[PRIV_IVAR:.*]] : !fir.ref<i32>)
7475
! CHECK-NEXT: omp.simd
75-
! CHECK-SAME: private(@[[PRIV_BOX_SYM:.*]] %{{.*}} -> %[[PRIV_BOX:.*]], @[[PRIV_IVAR_SYM:.*]] %{{.*}} -> %[[PRIV_IVAR:.*]] : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<i32>)
7676
! CHECK-NEXT: omp.loop_nest (%[[IVAR:.*]]) : i32
7777
!$omp do simd private(tmp)
7878
do i=1, 10
79-
! CHECK: %[[PRIV_BOX_DECL:.*]]:2 = hlfir.declare %[[PRIV_BOX]]
8079
! CHECK: %[[PRIV_IVAR_DECL:.*]]:2 = hlfir.declare %[[PRIV_IVAR]]
8180
! CHECK: hlfir.assign %[[IVAR]] to %[[PRIV_IVAR_DECL]]#0
82-
! CHECK: %[[PRIV_BOX_LOAD:.*]] = fir.load %[[PRIV_BOX_DECL]]
83-
! CHECK: hlfir.assign %{{.*}} to %[[PRIV_BOX_DECL]]#0
8481
! CHECK: omp.yield
8582
tmp = tmp + 1
8683
end do
@@ -90,13 +87,11 @@ end subroutine do_simd_private
9087
subroutine do_simd_lastprivate_firstprivate()
9188
integer :: a
9289
! CHECK: omp.wsloop
93-
! CHECK-SAME: private(@[[FIRSTPRIVATE_A_SYM:.*]] %{{.*}} -> %[[FIRSTPRIVATE_A:.*]] : !fir.ref<i32>)
90+
! CHECK-SAME: private(@[[FIRSTPRIVATE_A_SYM:.*]] %{{.*}} -> %[[FIRSTPRIVATE_A:.*]], @[[PRIVATE_I_SYM:.*]] %{{.*}} -> %[[PRIVATE_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
9491
! CHECK-NEXT: omp.simd
95-
! CHECK-SAME: private(@[[PRIVATE_A_SYM:.*]] %{{.*}} -> %[[PRIVATE_A:.*]], @[[PRIVATE_I_SYM:.*]] %{{.*}} -> %[[PRIVATE_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
9692
!$omp do simd lastprivate(a) firstprivate(a)
9793
do i = 1, 10
9894
! CHECK: %[[FIRSTPRIVATE_A_DECL:.*]]:2 = hlfir.declare %[[FIRSTPRIVATE_A]]
99-
! CHECK: %[[PRIVATE_A_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_A]]
10095
! CHECK: %[[PRIVATE_I_DECL:.*]]:2 = hlfir.declare %[[PRIVATE_I]]
10196
a = a + 1
10297
end do

0 commit comments

Comments
 (0)