[flang][OpenMP] Fix firstprivate not working with lastprivate in DO SIMD

[KrxGu]

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

Fixes #168306

[llvmbot]

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Krish Gupta (KrxGu)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/170163.diff

3 Files Affected:

• (modified) flang/lib/Lower/OpenMP/OpenMP.cpp (+1-1)
• (added) flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90 (+46)
• (added) flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90 (+87)

diff --git a/flang/lib/Lower/OpenMP/OpenMP.cpp b/flang/lib/Lower/OpenMP/OpenMP.cpp
index 6ca8636bb6459..356076e797f29 100644
--- a/flang/lib/Lower/OpenMP/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP/OpenMP.cpp
@@ -3324,7 +3324,7 @@ static mlir::omp::WsloopOp genCompositeDoSimd(
   genLoopNestOp(converter, symTable, semaCtx, eval, loc, queue, simdItem,
                 loopNestClauseOps, iv,
                 {{wsloopOp, wsloopArgs}, {simdOp, simdArgs}},
-                llvm::omp::Directive::OMPD_do_simd, simdItemDSP);
+                llvm::omp::Directive::OMPD_do_simd, wsloopItemDSP);
   return wsloopOp;
 }
 
diff --git a/flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90 b/flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90
new file mode 100644
index 0000000000000..a81640f8b1994
--- /dev/null
+++ b/flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90
@@ -0,0 +1,46 @@
+! Test runtime behavior of DO SIMD with firstprivate and lastprivate on same variable
+! This is the reproducer from issue #168306
+
+! RUN: %flang_fc1 -fopenmp -emit-llvm %s -o - | FileCheck %s --check-prefix=LLVM
+! RUN: %flang -fopenmp %s -o %t && %t | FileCheck %s
+
+! LLVM-LABEL: define {{.*}} @_QQmain
+program main
+  integer :: a
+  integer :: i
+  
+  a = 10
+  !$omp do simd lastprivate(a) firstprivate(a)
+  do i = 1, 1
+     ! Inside loop: a should be 10 (from firstprivate initialization)
+     ! CHECK: main1 : a = 10
+     print *, "main1 : a = ", a
+     a = 20
+  end do
+  !$omp end do simd
+  ! After loop: a should be 20 (from lastprivate copy-out)
+  ! CHECK: main2 : a = 20
+  print *, "main2 : a = ", a
+  
+  call sub
+  ! CHECK: pass
+  print *, 'pass'
+end program main
+
+subroutine sub
+  integer :: a
+  integer :: i
+  
+  a = 10
+  !$omp do simd lastprivate(a) firstprivate(a)
+  do i = 1, 1
+     ! Inside loop: a should be 10 (from firstprivate initialization)
+     ! CHECK: sub1  : a = 10
+     print *, "sub1  : a = ", a
+     a = 20
+  end do
+  !$omp end do simd
+  ! After loop: a should be 20 (from lastprivate copy-out)
+  ! CHECK: sub2  : a = 20
+  print *, "sub2  : a = ", a
+end subroutine sub
diff --git a/flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90 b/flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90
new file mode 100644
index 0000000000000..3f6b2ed817055
--- /dev/null
+++ b/flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90
@@ -0,0 +1,87 @@
+! Test for DO SIMD with the same variable in both firstprivate and lastprivate clauses
+! This tests the fix for issue #168306
+
+! RUN: %flang_fc1 -fopenmp -mmlir --openmp-enable-delayed-privatization-staging=true -emit-hlfir %s -o - | FileCheck %s
+
+! Test case 1: Basic test with firstprivate + lastprivate on same variable
+! CHECK-LABEL: func.func @_QPdo_simd_first_last_same_var
+subroutine do_simd_first_last_same_var()
+  integer :: a
+  integer :: i
+  a = 10
+
+  ! CHECK:      omp.wsloop
+  ! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]] : !fir.ref<i32>)
+  ! CHECK-NEXT: omp.simd
+  ! CHECK-SAME: private(@{{.*}} %{{.*}} -> %[[PRIV_A:.*]], @{{.*}} %{{.*}} -> %[[PRIV_I:.*]] : !fir.ref<i32>, !fir.ref<i32>)
+  ! CHECK-NEXT: omp.loop_nest (%[[IV:.*]]) : i32
+  !$omp do simd firstprivate(a) lastprivate(a)
+  do i = 1, 1
+    ! CHECK: %[[FIRSTPRIV_A_DECL:.*]]:2 = hlfir.declare %[[FIRSTPRIV_A]]
+    ! CHECK: %[[PRIV_A_DECL:.*]]:2 = hlfir.declare %[[PRIV_A]]
+    ! CHECK: %[[PRIV_I_DECL:.*]]:2 = hlfir.declare %[[PRIV_I]]
+    ! The private copy should be initialized from firstprivate (value 10)
+    ! and then modified to 20
+    a = 20
+  end do
+  !$omp end do simd
+  ! After the loop, 'a' should be 20 due to lastprivate
+end subroutine do_simd_first_last_same_var
+
+! Test case 2: Test with lastprivate and firstprivate in reverse order
+! CHECK-LABEL: func.func @_QPdo_simd_last_first_reverse
+subroutine do_simd_last_first_reverse()
+  integer :: a
+  integer :: i
+  a = 10
+
+  ! CHECK:      omp.wsloop
+  ! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]] : !fir.ref<i32>)
+  ! CHECK-NEXT: omp.simd
+  !$omp do simd lastprivate(a) firstprivate(a)
+  do i = 1, 1
+    a = 20
+  end do
+  !$omp end do simd
+end subroutine do_simd_last_first_reverse
+
+! Test case 3: Multiple variables with mixed privatization
+! CHECK-LABEL: func.func @_QPdo_simd_multiple_vars
+subroutine do_simd_multiple_vars()
+  integer :: a, b, c
+  integer :: i
+  a = 10
+  b = 20
+  c = 30
+
+  ! CHECK:      omp.wsloop
+  ! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %{{.*}}, @{{.*}}firstprivate{{.*}} %{{.*}} -> %{{.*}} : !fir.ref<i32>, !fir.ref<i32>)
+  ! CHECK-NEXT: omp.simd
+  !$omp do simd firstprivate(a, b) lastprivate(a) private(c)
+  do i = 1, 5
+    a = a + 1
+    b = b + 1
+    c = i
+  end do
+  !$omp end do simd
+end subroutine do_simd_multiple_vars
+
+! Test case 4: Reproducer from issue #168306
+! CHECK-LABEL: func.func @_QPissue_168306_reproducer
+subroutine issue_168306_reproducer()
+  integer :: a
+  integer :: i
+  a = 10
+
+  ! CHECK:      omp.wsloop
+  ! CHECK-SAME: private(@{{.*}}firstprivate{{.*}} %{{.*}} -> %[[FIRSTPRIV_A:.*]] : !fir.ref<i32>)
+  ! CHECK-NEXT: omp.simd
+  !$omp do simd lastprivate(a) firstprivate(a)
+  do i = 1, 1
+    ! Inside the loop, 'a' should start at 10 (from firstprivate)
+    ! This is the key behavior that was broken
+    a = 20
+  end do
+  !$omp end do simd
+  ! After the loop, 'a' should be 20 (from lastprivate)
+end subroutine issue_168306_reproducer

[github-actions]

🐧 Linux x64 Test Results

• 4102 tests passed
• 202 tests skipped

✅ The build succeeded and all tests passed.

[tblah]

I'm not sure about your interpretation of the spec. There is no firstprivate clause for the simd construct so the firstprivate cannot apply to the simd. Running flang without your patch it looks like we correctly apply firstprivate and lastprivate to the wsloop but only lastprivate to simd.

This results in LLVM codegen where there are three private variables: the firstprivate copy of a for omp do - this is unused and would be removed by the optimizer; the private copy of a used in the body of omp simd; and the private copy of the loop iteration variable. As you observed, this means that the value of a inside the loop is uninitialized memory.

If this is a bug then I think the bug is that there are two completely separate declarations and memory for a. This is still visible in your lit test.

[KrxGu]

I'm not sure about your interpretation of the spec. There is no firstprivate clause for the simd construct so the firstprivate cannot apply to the simd. Running flang without your patch it looks like we correctly apply firstprivate and lastprivate to the wsloop but only lastprivate to simd.

This results in LLVM codegen where there are three private variables: the firstprivate copy of a for omp do - this is unused and would be removed by the optimizer; the private copy of a used in the body of omp simd; and the private copy of the loop iteration variable. As you observed, this means that the value of a inside the loop is uninitialized memory.

If this is a bug then I think the bug is that there are two completely separate declarations and memory for a. This is still visible in your lit test.

Thanks - that makes sense. I agree firstprivate isn’t a clause on simd, so the goal shouldn’t be to “apply firstprivate to simd”. The real issue is that our lowering materializes two independent private storages for a (firstprivate on wsloop and lastprivate/private on simd), and the loop body ends up using the simd one (uninitialized). I’ll rework the patch so omp.simd reuses the wsloop privatization for list items already privatized by wsloop (i.e. no second omp.private/alloca for a) and adjust the lit test accordingly. Does that match the intended approach?

[tblah]

Okay I had a look at some other compilers on compiler explorer

• gfortran refuses to compile this
• ifx does print 10 (at -O0, there seems to be a bug at O3)
• classic flang (nvfortran) prints 10

The real issue is that our lowering materializes two independent private storages for a (firstprivate on wsloop and lastprivate/private on simd), and the loop body ends up using the simd one (uninitialized)

Pretty much. There shouldn't be two different private allocations. There should be only one, and that should be treated as firstprivate on the wsloop.

[Copilot]

Pull request overview

This PR fixes a bug where the firstprivate clause was being ignored when combined with lastprivate on the same variable in OpenMP do simd constructs. The bug caused private variables to remain uninitialized instead of being initialized from the original variable's value.

• Modified genCompositeDoSimd() to use the worksharing loop's DataSharingProcessor for privatization instead of the SIMD construct's processor
• Changed shouldCollectPreDeterminedSymbols parameter from false to true for the worksharing loop processor
• Added comprehensive lowering and runtime tests to verify the fix

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
flang/lib/Lower/OpenMP/OpenMP.cpp	Fixed privatization handling in genCompositeDoSimd() to correctly process firstprivate+lastprivate clauses by using wsloopItemDSP instead of simdItemDSP
flang/test/Lower/OpenMP/do-simd-firstprivate-lastprivate.f90	Added lowering tests verifying correct MLIR generation for firstprivate+lastprivate combinations in do simd constructs
flang/test/Integration/OpenMP/do-simd-firstprivate-lastprivate-runtime.f90	Added runtime tests verifying correct initialization and copy-out behavior for firstprivate+lastprivate variables

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[KrxGu]

Okay I had a look at some other compilers on compiler explorer

• gfortran refuses to compile this
• ifx does print 10 (at -O0, there seems to be a bug at O3)
• classic flang (nvfortran) prints 10

The real issue is that our lowering materializes two independent private storages for a (firstprivate on wsloop and lastprivate/private on simd), and the loop body ends up using the simd one (uninitialized)

Pretty much. There shouldn't be two different private allocations. There should be only one, and that should be treated as firstprivate on the wsloop.

I've addressed the duplicate privatization issue. The fix now ensures that only the wsloop creates private allocations for data-sharing clauses, and the simd reuses them without creating duplicates. This matches the pattern used in distribute_parallel_do composites.

I've also updated the affected tests to reflect the corrected behavior.

CI all green, Ready for merge when you have a chance to review.

[tblah]

Thanks for the update. LG

[KrxGu]

@tblah , Added CHECK-NOT: private after each [omp.simd] check to explicitly verify no privatization on simd.
Thanks for the suggestion!
CI all green, Good To Merge!!

[jeanPerier]

Hi @tblah and @KrxGu, can you consider a revert of this patch, it is crashing the compiler in some tests:

https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/0166/0166_0005.f90

flang -fopenmp -c rep.f90
flang: /llvm/include/llvm/ADT/STLExtras.h:841: llvm::detail::zippy<llvm::detail::zip_first, T, U, Args ...> llvm::zip_equal(T&&, U&&, Args&& ...) [with T = llvm::ArrayRef<mlir::BlockArgument>; U = llvm::SmallVector<mlir::Value>&; Args = {llvm::SmallVector<mlir::Value, 6>&}]: Assertion `all_equal({range_size(t), range_size(u), range_size(args)...}) && "Iteratees do not have equal length"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace and instructions to reproduce the bug.
Stack dump:
0.	Program arguments: flang -fc1 -triple x86_64-unknown-linux-gnu -emit-obj -fcolor-diagnostics -mrelocation-model pic -pic-level 2 -pic-is-pie -target-cpu x86-64 -fopenmp -resource-dir lib/clang/22 -mframe-pointer=all -o rep.o -x f95 rep.f90
 #0 0x0000000005528da8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (flang+0x5528da8)
 #1 0x0000000005525c44 SignalHandler(int, siginfo_t*, void*) Signals.cpp:0:0
 #2 0x00007f87971b6990 __restore_rt (/lib64/libpthread.so.0+0x12990)
 #3 0x00007f8795f9052f raise (/lib64/libc.so.6+0x4e52f)
 #4 0x00007f8795f63e65 abort (/lib64/libc.so.6+0x21e65)
 #5 0x00007f8795f63d39 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21d39)
 #6 0x00007f8795f88e86 (/lib64/libc.so.6+0x46e86)
 #7 0x0000000005e8068f Fortran::lower::omp::DataSharingProcessor::insertLastPrivateCompare(mlir::Operation*) (flang+0x5e8068f)

This is hitting other fujistsu tests.

[tblah]

Thanks for the ping Jean, I've reverted.

[eugeneepshteyn]

Thank you for reverting the commit, it helps to keep our testing sane.

Would it be possible to start running llvm-test-suite and also Fujitsu tests as part of pre-commit testing? This would help to catch any issues earlier in the process. The instructions for Fujitsu tests are here: https://github.com/fujitsu/compiler-test-suite/blob/main/RUN.md

P.S.: this is the full list of Fujitsu tests that failed this time: 0166_0005 0166_0006 0166_0008 0212_0008 0502_0001 0502_0002 0521_0004 1052_0147 1052_0148 1052_0149

[tblah]

I think they take order(s) of magnitude more compute than running llvm's built in lit and unit tests. I don't think it would be impossible but there could be pushback. But in an ideal world I would like to see this too.

Last I checked, for the Fujitsu tests there's no upstream public list of what is expected to pass and fail (and the few that are known to be flaky). When I run them myself I have to compare results from before and after a change to see if anything regressed. Having something with a straightforward pass/fail for the fujitsu tests would be needed. If you'd like to upstream that I would be more than happy to help testing where I can as it would make my work a bit easier.