[Flang][OpenMP] Fix bug with default(none) and host-assoc threadprivate variable #134122

mjklemm · 2025-04-02T17:22:34Z

When a host associated threadprivate variable was used in a parallel region with default(none) in an internal subroutine was failing, because the compiler did not properly determine that the variable was pre-determined threadprivate and thus should not have been reported as missing a DSA.

Copilot

Pull Request Overview

This PR fixes an issue where a host-associated threadprivate variable was incorrectly reported as missing a DSA in parallel regions with default(none).

Replace the direct threadprivate flag check with a check on the ultimate symbol to correctly determine threadprivateness.
Ensures that the compiler no longer flags pre-determined threadprivate variables erroneously.

Files not reviewed (1)

flang/test/Lower/OpenMP/threadprivate-host-association-3.f90: Language not supported

Comments suppressed due to low confidence (1)

flang/lib/Semantics/resolve-directives.cpp:2304

Ensure that GetUltimate() is guaranteed to return a valid symbol to avoid potential null dereferences.

!symbol->GetUltimate().test(Symbol::Flag::OmpThreadprivate) &&

llvmbot · 2025-04-02T17:23:07Z

@llvm/pr-subscribers-flang-semantics

@llvm/pr-subscribers-flang-openmp

Author: Michael Klemm (mjklemm)

Changes

When a host associated threadprivate variable was used in a parallel region with default(none) in an internal subroutine was failing, because the compiler did not properly determine that the variable was pre-determined threadprivate and thus should not have been reported as missing a DSA.

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

2 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+1-1)
(added) flang/test/Lower/OpenMP/threadprivate-host-association-3.f90 (+44)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index f905da0a7239d..cc76abec56946 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2301,7 +2301,7 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
         if (symbol != found) {
           name.symbol = found; // adjust the symbol within region
         } else if (GetContext().defaultDSA == Symbol::Flag::OmpNone &&
-            !symbol->test(Symbol::Flag::OmpThreadprivate) &&
+            !symbol->GetUltimate().test(Symbol::Flag::OmpThreadprivate) &&
             // Exclude indices of sequential loops that are privatised in
             // the scope of the parallel region, and not in this scope.
             // TODO: check whether this should be caught in IsObjectWithDSA
diff --git a/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90 b/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90
new file mode 100644
index 0000000000000..22ee51f82bc0f
--- /dev/null
+++ b/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90
@@ -0,0 +1,44 @@
+! This test checks lowering of OpenMP Threadprivate Directive.
+! Test for threadprivate variable in host association.
+
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK: func.func @_QQmain() attributes {fir.bindc_name = "main"} {
+!CHECK:   %[[A:.*]] = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK:   %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {fortran_attrs = #fir.var_attrs<internal_assoc>, uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   %[[A_ADDR:.*]] = fir.address_of(@_QFEa) : !fir.ref<i32>
+!CHECK:   %[[TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:   %[[TP_A_DECL:.*]]:2 = hlfir.declare %[[TP_A]] {fortran_attrs = #fir.var_attrs<internal_assoc>, uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   fir.call @_QFPsub() fastmath<contract> : () -> ()
+!CHECK:   return
+!CHECK: }
+!CHECK: func.func private @_QFPsub() attributes {fir.host_symbol = {{.*}}, llvm.linkage = #llvm.linkage<internal>} {
+!CHECK:   %[[A:.*]] = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK:   %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   %[[A_ADDR:.*]] = fir.address_of(@_QFEa) : !fir.ref<i32>
+!CHECK:   %[[TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:   %[[TP_A_DECL:.*]]:2 = hlfir.declare %[[TP_A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   omp.parallel {
+!CHECK:     %[[PAR_TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:     %[[PAR_TP_A_DECL:.*]]:2 = hlfir.declare %[[PAR_TP_A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:     %{{.*}} = fir.load %[[PAR_TP_A_DECL]]#0 : !fir.ref<i32>
+!CHECK:     omp.terminator
+!CHECK:   }
+!CHECK:   return
+!CHECK: }
+!CHECK: fir.global internal @_QFEa : i32 {
+!CHECK:   %[[A:.*]] = fir.undefined i32
+!CHECK:   fir.has_value %[[A]] : i32
+!CHECK: }
+
+program main
+   integer :: a
+   !$omp threadprivate(a)
+   call sub()
+contains
+   subroutine sub()
+      !$omp parallel default(none)
+      print *, a
+      !$omp end parallel
+   end
+end

llvmbot · 2025-04-02T17:23:08Z

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

Author: Michael Klemm (mjklemm)

Changes

When a host associated threadprivate variable was used in a parallel region with default(none) in an internal subroutine was failing, because the compiler did not properly determine that the variable was pre-determined threadprivate and thus should not have been reported as missing a DSA.

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

2 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+1-1)
(added) flang/test/Lower/OpenMP/threadprivate-host-association-3.f90 (+44)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index f905da0a7239d..cc76abec56946 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -2301,7 +2301,7 @@ void OmpAttributeVisitor::Post(const parser::Name &name) {
         if (symbol != found) {
           name.symbol = found; // adjust the symbol within region
         } else if (GetContext().defaultDSA == Symbol::Flag::OmpNone &&
-            !symbol->test(Symbol::Flag::OmpThreadprivate) &&
+            !symbol->GetUltimate().test(Symbol::Flag::OmpThreadprivate) &&
             // Exclude indices of sequential loops that are privatised in
             // the scope of the parallel region, and not in this scope.
             // TODO: check whether this should be caught in IsObjectWithDSA
diff --git a/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90 b/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90
new file mode 100644
index 0000000000000..22ee51f82bc0f
--- /dev/null
+++ b/flang/test/Lower/OpenMP/threadprivate-host-association-3.f90
@@ -0,0 +1,44 @@
+! This test checks lowering of OpenMP Threadprivate Directive.
+! Test for threadprivate variable in host association.
+
+!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK: func.func @_QQmain() attributes {fir.bindc_name = "main"} {
+!CHECK:   %[[A:.*]] = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK:   %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {fortran_attrs = #fir.var_attrs<internal_assoc>, uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   %[[A_ADDR:.*]] = fir.address_of(@_QFEa) : !fir.ref<i32>
+!CHECK:   %[[TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:   %[[TP_A_DECL:.*]]:2 = hlfir.declare %[[TP_A]] {fortran_attrs = #fir.var_attrs<internal_assoc>, uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   fir.call @_QFPsub() fastmath<contract> : () -> ()
+!CHECK:   return
+!CHECK: }
+!CHECK: func.func private @_QFPsub() attributes {fir.host_symbol = {{.*}}, llvm.linkage = #llvm.linkage<internal>} {
+!CHECK:   %[[A:.*]] = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK:   %[[A_DECL:.*]]:2 = hlfir.declare %[[A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   %[[A_ADDR:.*]] = fir.address_of(@_QFEa) : !fir.ref<i32>
+!CHECK:   %[[TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:   %[[TP_A_DECL:.*]]:2 = hlfir.declare %[[TP_A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:   omp.parallel {
+!CHECK:     %[[PAR_TP_A:.*]] = omp.threadprivate %[[A_ADDR]] : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:     %[[PAR_TP_A_DECL:.*]]:2 = hlfir.declare %[[PAR_TP_A]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:     %{{.*}} = fir.load %[[PAR_TP_A_DECL]]#0 : !fir.ref<i32>
+!CHECK:     omp.terminator
+!CHECK:   }
+!CHECK:   return
+!CHECK: }
+!CHECK: fir.global internal @_QFEa : i32 {
+!CHECK:   %[[A:.*]] = fir.undefined i32
+!CHECK:   fir.has_value %[[A]] : i32
+!CHECK: }
+
+program main
+   integer :: a
+   !$omp threadprivate(a)
+   call sub()
+contains
+   subroutine sub()
+      !$omp parallel default(none)
+      print *, a
+      !$omp end parallel
+   end
+end

tblah

Thanks for the fix!

kiranchandramohan · 2025-04-03T12:12:40Z

A semantics test (debug-dump-symbols or debug-unparse-with-symbols) would be better here.

mjklemm · 2025-04-05T11:46:20Z

A semantics test (debug-dump-symbols or debug-unparse-with-symbols) would be better here.

Hm. I have copied one of the existing tests for host associated threadprivate variables. I'll have a look to see if I can extend these tests, too.

Would you oppose approving with these extensions?

kiranchandramohan · 2025-04-05T13:23:34Z

A semantics test (debug-dump-symbols or debug-unparse-with-symbols) would be better here.

Hm. I have copied one of the existing tests for host associated threadprivate variables. I'll have a look to see if I can extend these tests, too.

Would you oppose approving with these extensions?

A semantics change is best tested with a semantics test. I am fine with the extension. Approving in advance.

kiranchandramohan

LG.

mjklemm · 2025-04-07T15:21:03Z

Looking at making the semantics checks a separate PR now.

mjklemm added 2 commits April 2, 2025 19:05

Fix bug with default(none) and host-assoc threadprivate var

576791c

Add test that uses default(none) with host-assoc threadprivate var

03456dc

mjklemm requested review from Copilot, dpalermo, harishch4, kiranchandramohan, kparzysz, skatrak and tblah April 2, 2025 17:22

mjklemm self-assigned this Apr 2, 2025

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp flang:semantics labels Apr 2, 2025

Copilot AI reviewed Apr 2, 2025

View reviewed changes

tblah approved these changes Apr 3, 2025

View reviewed changes

kiranchandramohan approved these changes Apr 5, 2025

View reviewed changes

mjklemm merged commit 7fa388d into llvm:main Apr 7, 2025
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Flang][OpenMP] Fix bug with default(none) and host-assoc threadprivate variable #134122

[Flang][OpenMP] Fix bug with default(none) and host-assoc threadprivate variable #134122

Uh oh!

mjklemm commented Apr 2, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

llvmbot commented Apr 2, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 2, 2025

Uh oh!

tblah left a comment

Uh oh!

kiranchandramohan commented Apr 3, 2025

Uh oh!

mjklemm commented Apr 5, 2025

Uh oh!

kiranchandramohan commented Apr 5, 2025

Uh oh!

kiranchandramohan left a comment

Uh oh!

Uh oh!

mjklemm commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Flang][OpenMP] Fix bug with default(none) and host-assoc threadprivate variable #134122

[Flang][OpenMP] Fix bug with default(none) and host-assoc threadprivate variable #134122

Uh oh!

Conversation

mjklemm commented Apr 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

llvmbot commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 2, 2025

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

kiranchandramohan commented Apr 3, 2025

Uh oh!

mjklemm commented Apr 5, 2025

Uh oh!

kiranchandramohan commented Apr 5, 2025

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjklemm commented Apr 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Apr 2, 2025 •

edited

Loading