[Flang][OpenMP] Fix to variables not inheriting data sharing attributes

[harishch4]

When a default(none) clause exists and a threadprivate variable is used inside the construct, the variable does not inherit threadprivate behavior and throws the below error.

error: The DEFAULT(NONE) clause requires that 'a' must be listed in a data-sharing attribute clause

Added a condition to skip the error if it is a threadprivate variable.

Fixes: #49545

[llvmbot]

@llvm/pr-subscribers-flang-semantics
@llvm/pr-subscribers-flang-openmp

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

Author: None (harishch4)

Changes

When a default(none) clause exists and a threadprivate variable is used inside the construct, the variable does not inherit threadprivate behavior and throws the below error.

> error: The DEFAULT(NONE) clause requires that 'a' must be listed in a data-sharing attribute clause

Added a condition to skip the error if it is a threadprivate variable.

Fixes: #49545

---

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

2 Files Affected:

• (modified) flang/lib/Semantics/resolve-directives.cpp (+1)
• (added) flang/test/Lower/OpenMP/threadprivate-default-clause.f90 (+60)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 2c570bc3abeb20b..e19f68eefa28672 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -1954,6 +1954,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) &&
             // 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-default-clause.f90 b/flang/test/Lower/OpenMP/threadprivate-default-clause.f90
new file mode 100644
index 000000000000000..be07aeca2d5c32a
--- /dev/null
+++ b/flang/test/Lower/OpenMP/threadprivate-default-clause.f90
@@ -0,0 +1,60 @@
+! Simple test for lowering of OpenMP Threadprivate Directive with HLFIR.
+
+!RUN: %flang_fc1  -emit-hlfir -fopenmp %s -o - | FileCheck %s
+
+!CHECK-LABEL: func.func @_QPsub1() {
+!CHECK:     %[[A:.*]] = fir.address_of(@_QFsub1Ea) : !fir.ref<i32>
+!CHECK:     %[[A_DECL:.*]]:2 = hlfir.declare %[[A]]  {uniq_name = "_QFsub1Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:     %[[A_TP0:.*]] = omp.threadprivate %[[A_DECL]]#1 : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:     %[[A_CVT:.*]]:2 = hlfir.declare %[[A_TP0]] {uniq_name = "_QFsub1Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:     omp.parallel {
+!CHECK:       %[[A_TP:.*]] = omp.threadprivate %[[A_DECL]]#1 : !fir.ref<i32> -> !fir.ref<i32>
+!CHECK:       %[[A_TP_DECL:.*]]:2 = hlfir.declare %[[A_TP]] {uniq_name = "_QFsub1Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:       %[[TID:.*]] = fir.call @omp_get_thread_num() fastmath<contract> : () -> i32
+!CHECK:       hlfir.assign %[[TID]] to %[[A_TP_DECL]]#0 : i32, !fir.ref<i32>
+!CHECK:       omp.terminator
+!CHECK:     }
+
+subroutine sub1()
+   use omp_lib
+   integer, save :: a
+   !$omp threadprivate(a)
+   !$omp parallel default(none)
+   a = omp_get_thread_num()
+   !$omp end parallel
+end subroutine
+
+!CHECK-LABEL: func.func @_QPsub2() {
+!CHECK:    %[[BLK:.*]] = fir.address_of(@blk_) : !fir.ref<!fir.array<4xi8>>
+!CHECK:    %[[BLK_CVT:.*]] = fir.convert %[[BLK]] : (!fir.ref<!fir.array<4xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK:    %c0 = arith.constant 0 : index
+!CHECK:    %[[A_ADDR:.*]] = fir.coordinate_of %[[BLK_CVT]], %c0 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK:    %[[A_CVT:.*]] = fir.convert %[[A_ADDR]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK:    %[[A_DECL:.*]]:2 = hlfir.declare %[[A_CVT]] {uniq_name = "_QFsub2Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:    %[[A_TP0:.*]] = omp.threadprivate %[[BLK]] : !fir.ref<!fir.array<4xi8>> -> !fir.ref<!fir.array<4xi8>>
+!CHECK:    %[[A_TP0_CVT:.*]] = fir.convert %[[A_TP0]] : (!fir.ref<!fir.array<4xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK:    %c0_0 = arith.constant 0 : index
+!CHECK:    %[[A_TP0_ADDR:.*]] = fir.coordinate_of %[[A_TP0_CVT]], %c0_0 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK:    %[[A_TP0_ADDR_CVT:.*]] = fir.convert %[[A_TP0_ADDR]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK:    %[[A_TP0_DECL:.*]]:2 = hlfir.declare %[[A_TP0_ADDR_CVT]] {uniq_name = "_QFsub2Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:    omp.parallel {
+!CHECK:       %[[BLK_TP:.*]] = omp.threadprivate %[[BLK]] : !fir.ref<!fir.array<4xi8>> -> !fir.ref<!fir.array<4xi8>>
+!CHECK:       %[[BLK_TP_CVT:.*]] = fir.convert %[[BLK_TP]] : (!fir.ref<!fir.array<4xi8>>) -> !fir.ref<!fir.array<?xi8>>
+!CHECK:       %c0_1 = arith.constant 0 : index
+!CHECK:       %[[A_TP_ADDR:.*]] = fir.coordinate_of %[[BLK_TP_CVT]], %c0_1 : (!fir.ref<!fir.array<?xi8>>, index) -> !fir.ref<i8>
+!CHECK:       %[[A_TP_ADDR_CVT:.*]] = fir.convert %[[A_TP_ADDR]] : (!fir.ref<i8>) -> !fir.ref<i32>
+!CHECK:       %[[A_TP_DECL:.*]]:2 = hlfir.declare %[[A_TP_ADDR_CVT]] {uniq_name = "_QFsub2Ea"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:       %[[TID:.*]] = fir.call @omp_get_thread_num() fastmath<contract> : () -> i32
+!CHECK:       hlfir.assign %[[TID]] to %[[A_TP_DECL]]#0 : i32, !fir.ref<i32>
+!CHECK:       omp.terminator
+!CHECK:     }
+
+subroutine sub2()
+   use omp_lib
+   integer :: a
+   common/blk/a
+   !$omp threadprivate(/blk/)
+   !$omp parallel default(none)
+   a = omp_get_thread_num()
+   !$omp end parallel
+end subroutine

[mjklemm]

LGTM, thanks!

[mjklemm]

@kiranchandramohan It would be great if we could land this before 19.x is branched. Mind having a quick look?

[kiranchandramohan]

LG.

[kiranchandramohan]

Does it fix #49545 ?

I will have a look later to see whether this can be part of IsObjectWithDSA.

[harishch4]

Does it fix #49545 ?

Yes, It does.