[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized #147442

agozillon · 2025-07-08T01:07:59Z

Currently, we indicate to the runtime that implicit scalar captures are firstprivate (via map and
capture types), enough for the runtime trace to treat it as such, but we do not CodeGen the IR
in such a way that we can take full advantage of this aspect of the OpenMP specification.

This patch seeks to change that by applying the correct symbol flags (firstprivate/implicit) to the
implicitly captured scalars within target regions, which then triggers the delayed privitization code
generation for these symbols, bringing the code generation in-line with the explicit firstpriviate
clause. Currently, similarly to the delayed privitization I have sheltered this segment of code
behind the EnabledDelayedPrivitization flag, as without it, we'll trigger an compiler error for
firstprivate not being supported any time we implicitly capture a scalar and try to firstprivitize
it, in future when this flag is removed it can also be removed here. So, for now, you need to
enable this via providing the compiler the flag on compilation of any programs.

llvmbot · 2025-07-08T01:08:27Z

@llvm/pr-subscribers-flang-openmp

@llvm/pr-subscribers-flang-semantics

Author: None (agozillon)

Changes

Currently, we indicate to the runtime that implicit scalar captures are firstprivate (via map and
capture types), enough for the runtime trace to treat it as such, but we do not CodeGen the IR
in such a way that we can take full advantage of this aspect of the OpenMP specification.

This patch seeks to change that by applying the correct symbol flags (firstprivate/implicit) to the
implicitly captured scalars within target regions, which then triggers the delayed privitization code
generation for these symbols, bringing the code generation in-line with the explicit firstpriviate
clause. Currently, similarly to the delayed privitization I have sheltered this segment of code
behind the EnabledDelayedPrivitization flag, as without it, we'll trigger an compiler error for
firstprivate not being supported any time we implicitly capture a scalar and try to firstprivitize
it, in future when this flag is removed it can also be removed here. So, for now, you need to
enable this via providing the compiler the flag on compilation of any programs.

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

3 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+38-1)
(added) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90 (+42)
(modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 (+8-8)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 299bb6ff876e7..f7aa2df4c25b1 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -23,6 +23,7 @@
 #include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
+#include "flang/Support/Flags.h"
 #include "llvm/Frontend/OpenMP/OMP.h.inc"
 #include "llvm/Support/Debug.h"
 #include <list>
@@ -2297,6 +2298,34 @@ static bool IsSymbolStaticStorageDuration(const Symbol &symbol) {
       (ultSym.flags().test(Symbol::Flag::InCommonBlock));
 }
 
+static bool IsTargetCaptureImplicitlyFirstPrivatizeable(
+    const Symbol &symbol, const Symbol::Flags &dsa) {
+  // if we're associated with any other flags we skip implicit privitization
+  // for now. If we're an allocatable, pointer or declare target, we're not
+  // implicitly firstprivitizeable under OpenMP restrictions.
+  // TODO: Relax restriction as we progress privitization and further
+  // investigate the flags we can intermix with.
+  if (!dsa.none() || !symbol.flags().none() ||
+      semantics::IsAllocatableOrPointer(symbol)) {
+    return false;
+  }
+
+  // It is default firstprivatizeable as far as the OpenMP specification is
+  // concerned if it is a non-array scalar type that has been implicitly
+  // captured in a target region
+  const auto *type{symbol.GetType()};
+  if ((!symbol.GetShape() || symbol.GetShape()->empty()) &&
+      (type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Numeric ||
+          type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Logical ||
+          type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Character)) {
+    return true;
+  }
+  return false;
+}
+
 void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
   if (!IsPrivatizable(symbol)) {
     return;
@@ -2444,7 +2473,15 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
       useLastDeclSymbol();
       PRINT_IMPLICIT_RULE("3) enclosing context");
     } else if (targetDir) {
-      // TODO 4) not mapped target variable -> firstprivate
+      // 4) not mapped target variable  -> firstprivate
+      //    - i.e. implicit, but meets OpenMP specification rules for
+      //    firstprivate "promotion"
+      if (enableDelayedPrivatizationStaging && symbol &&
+          IsTargetCaptureImplicitlyFirstPrivatizeable(*symbol, prevDSA)) {
+        prevDSA.set(Symbol::Flag::OmpImplicit);
+        prevDSA.set(Symbol::Flag::OmpFirstPrivate);
+        makeSymbol(prevDSA);
+      }
       dsa = prevDSA;
     } else if (taskGenDir) {
       // TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90
new file mode 100644
index 0000000000000..6cd14e7d3a2b4
--- /dev/null
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90
@@ -0,0 +1,42 @@
+! Tests delayed privatization works for implicit capture of scalars similarly to
+! the way it works for explicitly firstprivitized scalars.
+
+! RUN: %flang_fc1 -emit-mlir -fopenmp -mmlir --enable-delayed-privatization-staging \
+! RUN:   -o - %s 2>&1 | FileCheck %s
+
+! CHECK-LABEL:   omp.private {type = firstprivate} @_QFExdgfx_firstprivate_i32 : i32 copy {
+! CHECK:         ^bb0(%{{.*}}: !fir.ref<i32>, %{{.*}}: !fir.ref<i32>):
+! CHECK:           %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+! CHECK:           fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+! CHECK:           omp.yield(%{{.*}} : !fir.ref<i32>)
+! CHECK:         }
+
+! CHECK-LABEL:   omp.private {type = firstprivate} @_QFExfpvx_firstprivate_i32 : i32 copy {
+! CHECK:         ^bb0(%{{.*}}: !fir.ref<i32>, %{{.*}}: !fir.ref<i32>):
+! CHECK:           %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+! CHECK:           fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+! CHECK:           omp.yield(%{{.*}} : !fir.ref<i32>)
+! CHECK:         }
+
+! CHECK:  %[[VAL_0:.*]] = fir.alloca i32 {bindc_name = "xdgfx", uniq_name = "_QFExdgfx"}
+! CHECK:  %[[VAL_1:.*]] = fir.declare %[[VAL_0]] {uniq_name = "_QFExdgfx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %[[VAL_2:.*]] = fir.alloca i32 {bindc_name = "xfpvx", uniq_name = "_QFExfpvx"}
+! CHECK:  %[[VAL_3:.*]] = fir.declare %[[VAL_2]] {uniq_name = "_QFExfpvx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %[[VAL_4:.*]] = omp.map.info var_ptr(%[[VAL_3]] : !fir.ref<i32>, i32) map_clauses(to) capture(ByRef) -> !fir.ref<i32>
+! CHECK:  %[[VAL_5:.*]] = omp.map.info var_ptr(%[[VAL_1]] : !fir.ref<i32>, i32) map_clauses(to) capture(ByRef) -> !fir.ref<i32>
+
+! CHECK:  omp.target map_entries(%[[VAL_4]] -> %{{.*}}, %[[VAL_5]] -> %{{.*}} : !fir.ref<i32>, !fir.ref<i32>) private(@_QFExfpvx_firstprivate_i32 %[[VAL_3]] -> %[[VAL_6:.*]] [map_idx=0], @_QFExdgfx_firstprivate_i32 %[[VAL_1]] -> %[[VAL_7:.*]] [map_idx=1] : !fir.ref<i32>, !fir.ref<i32>) {
+! CHECK:  %{{.*}} = fir.declare %[[VAL_6]] {uniq_name = "_QFExfpvx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %{{.*}} = fir.declare %[[VAL_7]] {uniq_name = "_QFExdgfx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+
+program test_default_implicit_firstprivate
+  implicit none
+  integer :: xdgfx, xfpvx
+  xdgfx = 1
+  xfpvx = 2
+  !$omp target firstprivate(xfpvx)
+  xdgfx = 42
+  xfpvx = 43
+  !$omp end target
+  write(*,*) xdgfx, xfpvx
+end program
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
index 217ac5638a3ea..e7e3fb2097308 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
@@ -139,28 +139,28 @@ end subroutine target_allocatable
 ! CHECK:        %[[REAL_ARR_ALLOC:.*]] = fir.alloca !fir.array<?xf32>, {{.*}} {bindc_name = "real_arr", {{.*}}}
 ! CHECK:        %[[REAL_ARR_DECL:.*]]:2 = hlfir.declare %[[REAL_ARR_ALLOC]]({{.*}})
 ! CHECK:        fir.store %[[REAL_ARR_DECL]]#0 to %[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>
-! CHECK:        %[[MAPPED_MI0:.*]] = omp.map.info var_ptr(%[[MAPPED_DECL]]#1 : !fir.ref<i32>, i32) {{.*}}
 ! CHECK:        %[[ALLOC_VAR_MEMBER:.*]] = omp.map.info var_ptr(%[[ALLOC_VAR_DECL]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>, i32)
 ! CHECK:        %[[ALLOC_VAR_MAP:.*]] = omp.map.info var_ptr(%[[ALLOC_VAR_DECL]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.box<!fir.heap<i32>>) {{.*}} members(%[[ALLOC_VAR_MEMBER]] :
 ! CHECK:        %[[REAL_ARR_MEMBER:.*]] = omp.map.info var_ptr(%[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>, f32)
 ! CHECK:        %[[REAL_ARR_DESC_MAP:.*]] = omp.map.info var_ptr(%[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.box<!fir.array<?xf32>>) {{.*}} members(%[[REAL_ARR_MEMBER]] :
 ! CHECK:        fir.store %[[CHAR_VAR_DECL]]#0 to %[[CHAR_VAR_DESC_ALLOCA]] : !fir.ref<!fir.boxchar<1>>
 ! CHECK:        %[[CHAR_VAR_DESC_MAP:.*]] = omp.map.info var_ptr(%[[CHAR_VAR_DESC_ALLOCA]] : !fir.ref<!fir.boxchar<1>>, !fir.boxchar<1>)
+! CHECK:        %[[MAPPED_MI0:.*]] = omp.map.info var_ptr(%[[MAPPED_DECL]]#0 : !fir.ref<i32>, i32) {{.*}}
 ! CHECK:        omp.target
 ! CHECK-SAME:     map_entries(
-! CHECK-SAME:       %[[MAPPED_MI0]] -> %[[MAPPED_ARG0:[^,]+]],
-! CHECK-SAME:       %[[ALLOC_VAR_MAP]] -> %[[MAPPED_ARG1:[^,]+]]
+! CHECK-SAME:       %[[ALLOC_VAR_MAP]] -> %[[MAPPED_ARG1:[^,]+]],
 ! CHECK-SAME:       %[[REAL_ARR_DESC_MAP]] -> %[[MAPPED_ARG2:[^,]+]]
 ! CHECK-SAME:       %[[CHAR_VAR_DESC_MAP]] -> %[[MAPPED_ARG3:.[^,]+]]
-! CHECK-SAME:       !fir.ref<i32>, !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<!fir.boxchar<1>>, !fir.llvm_ptr<!fir.ref<i32>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>
+! CHECK-SAME:       %[[MAPPED_MI0]] -> %[[MAPPED_ARG0:[^,]+]]
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<!fir.boxchar<1>>, !fir.ref<i32>, !fir.llvm_ptr<!fir.ref<i32>>
 ! CHECK-SAME:     private(
-! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=1],
+! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=0],
 ! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:[^,]+]],
 ! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:[^,]+]],
-! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}} -> %[[ARR_ARG:[^,]+]] [map_idx=2],
+! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}} -> %[[ARR_ARG:[^,]+]] [map_idx=1],
 ! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:[^,]+]],
-! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] [map_idx=3] :
-! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<complex<f32>>, !fir.boxchar<1>) {
+! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] [map_idx=2],
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<complex<f32>>, !fir.boxchar<1>, !fir.ref<i32>) {
 ! CHECK-NOT:      fir.alloca
 ! CHECK:          hlfir.declare %[[ALLOC_ARG]]
 ! CHECK:          hlfir.declare %[[REAL_ARG]]

llvmbot · 2025-07-08T01:08:28Z

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

Author: None (agozillon)

Changes

Currently, we indicate to the runtime that implicit scalar captures are firstprivate (via map and
capture types), enough for the runtime trace to treat it as such, but we do not CodeGen the IR
in such a way that we can take full advantage of this aspect of the OpenMP specification.

This patch seeks to change that by applying the correct symbol flags (firstprivate/implicit) to the
implicitly captured scalars within target regions, which then triggers the delayed privitization code
generation for these symbols, bringing the code generation in-line with the explicit firstpriviate
clause. Currently, similarly to the delayed privitization I have sheltered this segment of code
behind the EnabledDelayedPrivitization flag, as without it, we'll trigger an compiler error for
firstprivate not being supported any time we implicitly capture a scalar and try to firstprivitize
it, in future when this flag is removed it can also be removed here. So, for now, you need to
enable this via providing the compiler the flag on compilation of any programs.

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

3 Files Affected:

(modified) flang/lib/Semantics/resolve-directives.cpp (+38-1)
(added) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90 (+42)
(modified) flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 (+8-8)

diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 299bb6ff876e7..f7aa2df4c25b1 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -23,6 +23,7 @@
 #include "flang/Semantics/openmp-modifiers.h"
 #include "flang/Semantics/symbol.h"
 #include "flang/Semantics/tools.h"
+#include "flang/Support/Flags.h"
 #include "llvm/Frontend/OpenMP/OMP.h.inc"
 #include "llvm/Support/Debug.h"
 #include <list>
@@ -2297,6 +2298,34 @@ static bool IsSymbolStaticStorageDuration(const Symbol &symbol) {
       (ultSym.flags().test(Symbol::Flag::InCommonBlock));
 }
 
+static bool IsTargetCaptureImplicitlyFirstPrivatizeable(
+    const Symbol &symbol, const Symbol::Flags &dsa) {
+  // if we're associated with any other flags we skip implicit privitization
+  // for now. If we're an allocatable, pointer or declare target, we're not
+  // implicitly firstprivitizeable under OpenMP restrictions.
+  // TODO: Relax restriction as we progress privitization and further
+  // investigate the flags we can intermix with.
+  if (!dsa.none() || !symbol.flags().none() ||
+      semantics::IsAllocatableOrPointer(symbol)) {
+    return false;
+  }
+
+  // It is default firstprivatizeable as far as the OpenMP specification is
+  // concerned if it is a non-array scalar type that has been implicitly
+  // captured in a target region
+  const auto *type{symbol.GetType()};
+  if ((!symbol.GetShape() || symbol.GetShape()->empty()) &&
+      (type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Numeric ||
+          type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Logical ||
+          type->category() ==
+              Fortran::semantics::DeclTypeSpec::Category::Character)) {
+    return true;
+  }
+  return false;
+}
+
 void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
   if (!IsPrivatizable(symbol)) {
     return;
@@ -2444,7 +2473,15 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
       useLastDeclSymbol();
       PRINT_IMPLICIT_RULE("3) enclosing context");
     } else if (targetDir) {
-      // TODO 4) not mapped target variable -> firstprivate
+      // 4) not mapped target variable  -> firstprivate
+      //    - i.e. implicit, but meets OpenMP specification rules for
+      //    firstprivate "promotion"
+      if (enableDelayedPrivatizationStaging && symbol &&
+          IsTargetCaptureImplicitlyFirstPrivatizeable(*symbol, prevDSA)) {
+        prevDSA.set(Symbol::Flag::OmpImplicit);
+        prevDSA.set(Symbol::Flag::OmpFirstPrivate);
+        makeSymbol(prevDSA);
+      }
       dsa = prevDSA;
     } else if (taskGenDir) {
       // TODO 5) dummy arg in orphaned taskgen construct -> firstprivate
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90
new file mode 100644
index 0000000000000..6cd14e7d3a2b4
--- /dev/null
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90
@@ -0,0 +1,42 @@
+! Tests delayed privatization works for implicit capture of scalars similarly to
+! the way it works for explicitly firstprivitized scalars.
+
+! RUN: %flang_fc1 -emit-mlir -fopenmp -mmlir --enable-delayed-privatization-staging \
+! RUN:   -o - %s 2>&1 | FileCheck %s
+
+! CHECK-LABEL:   omp.private {type = firstprivate} @_QFExdgfx_firstprivate_i32 : i32 copy {
+! CHECK:         ^bb0(%{{.*}}: !fir.ref<i32>, %{{.*}}: !fir.ref<i32>):
+! CHECK:           %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+! CHECK:           fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+! CHECK:           omp.yield(%{{.*}} : !fir.ref<i32>)
+! CHECK:         }
+
+! CHECK-LABEL:   omp.private {type = firstprivate} @_QFExfpvx_firstprivate_i32 : i32 copy {
+! CHECK:         ^bb0(%{{.*}}: !fir.ref<i32>, %{{.*}}: !fir.ref<i32>):
+! CHECK:           %{{.*}} = fir.load %{{.*}} : !fir.ref<i32>
+! CHECK:           fir.store %{{.*}} to %{{.*}} : !fir.ref<i32>
+! CHECK:           omp.yield(%{{.*}} : !fir.ref<i32>)
+! CHECK:         }
+
+! CHECK:  %[[VAL_0:.*]] = fir.alloca i32 {bindc_name = "xdgfx", uniq_name = "_QFExdgfx"}
+! CHECK:  %[[VAL_1:.*]] = fir.declare %[[VAL_0]] {uniq_name = "_QFExdgfx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %[[VAL_2:.*]] = fir.alloca i32 {bindc_name = "xfpvx", uniq_name = "_QFExfpvx"}
+! CHECK:  %[[VAL_3:.*]] = fir.declare %[[VAL_2]] {uniq_name = "_QFExfpvx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %[[VAL_4:.*]] = omp.map.info var_ptr(%[[VAL_3]] : !fir.ref<i32>, i32) map_clauses(to) capture(ByRef) -> !fir.ref<i32>
+! CHECK:  %[[VAL_5:.*]] = omp.map.info var_ptr(%[[VAL_1]] : !fir.ref<i32>, i32) map_clauses(to) capture(ByRef) -> !fir.ref<i32>
+
+! CHECK:  omp.target map_entries(%[[VAL_4]] -> %{{.*}}, %[[VAL_5]] -> %{{.*}} : !fir.ref<i32>, !fir.ref<i32>) private(@_QFExfpvx_firstprivate_i32 %[[VAL_3]] -> %[[VAL_6:.*]] [map_idx=0], @_QFExdgfx_firstprivate_i32 %[[VAL_1]] -> %[[VAL_7:.*]] [map_idx=1] : !fir.ref<i32>, !fir.ref<i32>) {
+! CHECK:  %{{.*}} = fir.declare %[[VAL_6]] {uniq_name = "_QFExfpvx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+! CHECK:  %{{.*}} = fir.declare %[[VAL_7]] {uniq_name = "_QFExdgfx"} : (!fir.ref<i32>) -> !fir.ref<i32>
+
+program test_default_implicit_firstprivate
+  implicit none
+  integer :: xdgfx, xfpvx
+  xdgfx = 1
+  xfpvx = 2
+  !$omp target firstprivate(xfpvx)
+  xdgfx = 42
+  xfpvx = 43
+  !$omp end target
+  write(*,*) xdgfx, xfpvx
+end program
diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
index 217ac5638a3ea..e7e3fb2097308 100644
--- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
+++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-multiple-variables.f90
@@ -139,28 +139,28 @@ end subroutine target_allocatable
 ! CHECK:        %[[REAL_ARR_ALLOC:.*]] = fir.alloca !fir.array<?xf32>, {{.*}} {bindc_name = "real_arr", {{.*}}}
 ! CHECK:        %[[REAL_ARR_DECL:.*]]:2 = hlfir.declare %[[REAL_ARR_ALLOC]]({{.*}})
 ! CHECK:        fir.store %[[REAL_ARR_DECL]]#0 to %[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>
-! CHECK:        %[[MAPPED_MI0:.*]] = omp.map.info var_ptr(%[[MAPPED_DECL]]#1 : !fir.ref<i32>, i32) {{.*}}
 ! CHECK:        %[[ALLOC_VAR_MEMBER:.*]] = omp.map.info var_ptr(%[[ALLOC_VAR_DECL]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>, i32)
 ! CHECK:        %[[ALLOC_VAR_MAP:.*]] = omp.map.info var_ptr(%[[ALLOC_VAR_DECL]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.box<!fir.heap<i32>>) {{.*}} members(%[[ALLOC_VAR_MEMBER]] :
 ! CHECK:        %[[REAL_ARR_MEMBER:.*]] = omp.map.info var_ptr(%[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>, f32)
 ! CHECK:        %[[REAL_ARR_DESC_MAP:.*]] = omp.map.info var_ptr(%[[REAL_ARR_DESC_ALLOCA]] : !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.box<!fir.array<?xf32>>) {{.*}} members(%[[REAL_ARR_MEMBER]] :
 ! CHECK:        fir.store %[[CHAR_VAR_DECL]]#0 to %[[CHAR_VAR_DESC_ALLOCA]] : !fir.ref<!fir.boxchar<1>>
 ! CHECK:        %[[CHAR_VAR_DESC_MAP:.*]] = omp.map.info var_ptr(%[[CHAR_VAR_DESC_ALLOCA]] : !fir.ref<!fir.boxchar<1>>, !fir.boxchar<1>)
+! CHECK:        %[[MAPPED_MI0:.*]] = omp.map.info var_ptr(%[[MAPPED_DECL]]#0 : !fir.ref<i32>, i32) {{.*}}
 ! CHECK:        omp.target
 ! CHECK-SAME:     map_entries(
-! CHECK-SAME:       %[[MAPPED_MI0]] -> %[[MAPPED_ARG0:[^,]+]],
-! CHECK-SAME:       %[[ALLOC_VAR_MAP]] -> %[[MAPPED_ARG1:[^,]+]]
+! CHECK-SAME:       %[[ALLOC_VAR_MAP]] -> %[[MAPPED_ARG1:[^,]+]],
 ! CHECK-SAME:       %[[REAL_ARR_DESC_MAP]] -> %[[MAPPED_ARG2:[^,]+]]
 ! CHECK-SAME:       %[[CHAR_VAR_DESC_MAP]] -> %[[MAPPED_ARG3:.[^,]+]]
-! CHECK-SAME:       !fir.ref<i32>, !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<!fir.boxchar<1>>, !fir.llvm_ptr<!fir.ref<i32>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>
+! CHECK-SAME:       %[[MAPPED_MI0]] -> %[[MAPPED_ARG0:[^,]+]]
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<!fir.boxchar<1>>, !fir.ref<i32>, !fir.llvm_ptr<!fir.ref<i32>>
 ! CHECK-SAME:     private(
-! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=1],
+! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=0],
 ! CHECK-SAME:       @[[REAL_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[REAL_ARG:[^,]+]],
 ! CHECK-SAME:       @[[LB_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[LB_ARG:[^,]+]],
-! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}} -> %[[ARR_ARG:[^,]+]] [map_idx=2],
+! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}} -> %[[ARR_ARG:[^,]+]] [map_idx=1],
 ! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:[^,]+]],
-! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] [map_idx=3] :
-! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<complex<f32>>, !fir.boxchar<1>) {
+! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] [map_idx=2],
+! CHECK-SAME:       !fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<f32>, !fir.ref<i64>, !fir.ref<!fir.box<!fir.array<?xf32>>>, !fir.ref<complex<f32>>, !fir.boxchar<1>, !fir.ref<i32>) {
 ! CHECK-NOT:      fir.alloca
 ! CHECK:          hlfir.declare %[[ALLOC_ARG]]
 ! CHECK:          hlfir.declare %[[REAL_ARG]]

agozillon · 2025-07-08T01:09:18Z

Please feel free to add more people to the review list (or add/remove yourself if you wish), wasn't sure who were the best people to review this as it's not my usual little segment!

flang/lib/Semantics/resolve-directives.cpp

agozillon · 2025-07-08T21:56:05Z

Thank you very much for the quick review @luporl ! Ill update this PR in the next week or so with the review points.

github-actions · 2025-07-24T21:51:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

agozillon · 2025-07-24T21:53:10Z

I've updated the PR with the current review comments (where still relevant), but the PR has undergone a fairly chunky set of additions to support the implicit firstprivitization of scalars in target teams distribute parallel do and associated friends, as the last change was only applying to target, but it should extend to all target modified constructs/directives!

But it is now ready for a subsequent review if anyone could please spare some time :-)

bhandarkar-pranav

Largely LGTM. I have a few minor comments though.

bhandarkar-pranav · 2025-07-25T20:34:08Z

flang/lib/Lower/OpenMP/DataSharingProcessor.h

@@ -93,6 +93,7 @@ class DataSharingProcessor {
  bool useDelayedPrivatization;
  llvm::SmallSet<const semantics::Symbol *, 16> mightHaveReadHostSym;
  lower::SymMap &symTable;
+  bool isTargetPrivitization;


nit: s/isTargetPrivitization/isTargetPrivatization

bhandarkar-pranav · 2025-07-25T20:34:24Z

flang/lib/Lower/OpenMP/DataSharingProcessor.h


  DataSharingProcessor(lower::AbstractConverter &converter,
                       semantics::SemanticsContext &semaCtx,
                       lower::pft::Evaluation &eval,
-                       bool useDelayedPrivatization, lower::SymMap &symTable);
+                       bool useDelayedPrivatization, lower::SymMap &symTable,
+                       bool isTargetPrivitization = false);


nit - spelling of privatization

bhandarkar-pranav · 2025-07-28T15:26:43Z

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

 DataSharingProcessor::DataSharingProcessor(
    lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
    const List<Clause> &clauses, lower::pft::Evaluation &eval,
    bool shouldCollectPreDeterminedSymbols, bool useDelayedPrivatization,
-    lower::SymMap &symTable)
+    lower::SymMap &symTable, bool isTargetPrivitization)


nit: Spelling of Privatization.

bhandarkar-pranav · 2025-07-28T15:48:58Z

flang/lib/Lower/OpenMP/OpenMP.cpp

+    if (llvm::is_contained(concatSyms, &checkSym))
+      return true;
+
+    return std::any_of(concatSyms.begin(), concatSyms.end(),


Again, a nit really. But, doens't this second check subsume the check above?

It might be equivalent due to the checkSymbol call at L2500, but without it I don't think it is, since we have L2500 though I'll see if removing it works fine! Thank you for spotting that.

bhandarkar-pranav · 2025-07-28T15:50:57Z

flang/lib/Lower/Support/Utils.cpp

@@ -12,6 +12,7 @@

 #include "flang/Lower/Support/Utils.h"

+#include "flang/Common/idioms.h"


Is this used?

bhandarkar-pranav · 2025-07-28T15:54:16Z

flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp

+    // Check if a value of type `type` can be passed to the kernel by value.
+    // All kernel parameters are of pointer type, so if the value can be
+    // represented inside of a pointer, then it can be passed by value.
+    auto isLiteralType = [&](mlir::Type type) {


Consider renaming the lambda to reflect that we really are checking if the range of a variable and not necessary literals only. For example, canPassByValue

bhandarkar-pranav · 2025-07-28T16:03:49Z

flang/lib/Semantics/resolve-directives.cpp

+    std::map<parser::OmpVariableCategory::Value,
+        parser::OmpDefaultmapClause::ImplicitBehavior>
+        defaultMap) {
+  if (!defaultMap.empty()) {


< minor >: Could you please add a comment here to suggest that we exit early if we are dealing with a scalar and the defaultmap clause has set the implicit mapping behavior to something other than firstprivate.

Helps readability because the compound conditional is quite long.

bhandarkar-pranav · 2025-07-28T16:05:09Z

flang/lib/Semantics/resolve-directives.cpp

+      return false;
+    }
+
+    if (llvm::is_contained(


This check seems to subsume the check above. If that's correct, then we should lead with this check rather than the one before.

luporl

The changes in DataSharingProcessor and the creation of implicit symbols in resolve-directives.cpp looks good to me, thanks.

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

flang/lib/Semantics/resolve-directives.cpp

agozillon

I think the last update covers all prior review comments made, please do feel free to add more of course! I've also rebased it on top of a commit from today, apologies if that makes things a little difficult to review, I wanted to make sure I tested against something more recent!

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

agozillon · 2025-07-30T20:18:15Z

flang/lib/Lower/OpenMP/OpenMP.cpp

+    if (llvm::is_contained(concatSyms, &checkSym))
+      return true;
+
+    return std::any_of(concatSyms.begin(), concatSyms.end(),


It might be equivalent due to the checkSymbol call at L2500, but without it I don't think it is, since we have L2500 though I'll see if removing it works fine! Thank you for spotting that.

flang/lib/Semantics/resolve-directives.cpp

Currently, we indicate to the runtime that implicit scalar captures are firstprivate (via map and capture types), enough for the runtime trace to treat it as such, but we do not CodeGen the IR in such a way that we can take full advantage of this aspect of the OpenMP specification. This patch seeks to change that by applying the correct symbol flags (firstprivate/implicit) to the implicitly captured scalars within target regions, which then triggers the delayed privitization code generation for these symbols, bringing the code generation in-line with the explicit firstpriviate clause. Currently, similarly to the delayed privitization I have sheltered this segment of code behind the EnabledDelayedPrivitization flag, as without it, we'll trigger an compiler error for firstprivate not being supported any time we implicitly capture a scalar and try to firstprivitize it, in future when this flag is removed it can also be removed here. So, for now, you need to enable this via providing the compiler the flag on compilation of any programs.

luporl

LGTM, thanks for addressing all my comments.

agozillon requested review from skatrak, tblah, mjklemm, dpalermo, mrkajetanp, TIFitis and bhandarkar-pranav July 8, 2025 01:07

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

agozillon changed the title ~~[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized~~ [Flang][OpenMP] Make implicitly captured scalars fully firstprivatized Jul 8, 2025

luporl reviewed Jul 8, 2025

View reviewed changes

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved

flang/lib/Semantics/resolve-directives.cpp Show resolved Hide resolved

agozillon force-pushed the implicit-map-capture-firstprivate branch from 9314e8e to d16df10 Compare July 24, 2025 21:48

agozillon force-pushed the implicit-map-capture-firstprivate branch from d16df10 to eccbf9e Compare July 24, 2025 21:57

agozillon mentioned this pull request Jul 25, 2025

[flang][OpenMP][debug] Build failure with duplicate variable mapping #146847

Open

bhandarkar-pranav reviewed Jul 28, 2025

View reviewed changes

luporl reviewed Jul 28, 2025

View reviewed changes

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp Outdated Show resolved Hide resolved

flang/lib/Semantics/resolve-directives.cpp Outdated Show resolved Hide resolved

agozillon force-pushed the implicit-map-capture-firstprivate branch from eccbf9e to b414863 Compare July 30, 2025 21:23

agozillon commented Jul 30, 2025

View reviewed changes

agozillon force-pushed the implicit-map-capture-firstprivate branch from b414863 to 4c399c2 Compare July 31, 2025 13:21

luporl approved these changes Jul 31, 2025

View reviewed changes

		@@ -12,6 +12,7 @@

		#include "flang/Lower/Support/Utils.h"

		#include "flang/Common/idioms.h"

[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized #147442

Are you sure you want to change the base?

[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized #147442

Conversation

agozillon commented Jul 8, 2025

Uh oh!

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

Uh oh!

Uh oh!

llvmbot commented Jul 8, 2025

Uh oh!

agozillon commented Jul 8, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agozillon commented Jul 8, 2025

Uh oh!

github-actions bot commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agozillon commented Jul 24, 2025

Uh oh!

bhandarkar-pranav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luporl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agozillon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luporl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jul 8, 2025 •

edited

Loading

github-actions bot commented Jul 24, 2025 •

edited

Loading