Revert "[Flang][OpenMP] Make implicitly captured scalars fully firstprivatized (llvm#147442)"

This reverts commit cc2a385.

Author: ronlieb

flang/lib/Lower/OpenMP/DataSharingProcessor.cpp

@@ -46,9 +46,25 @@ bool DataSharingProcessor::OMPConstructSymbolVisitor::isSymbolDefineBy(
 static bool isConstructWithTopLevelTarget(lower::pft::Evaluation &eval) {
   const auto *ompEval = eval.getIf<parser::OpenMPConstruct>();
   if (ompEval) {
-    auto dir = parser::omp::GetOmpDirectiveName(ompEval).v;
-    if (llvm::omp::topTargetSet.test(dir))
+    auto dir = parser::omp::GetOmpDirectiveName(*ompEval).v;
+    switch (dir) {
+    case llvm::omp::Directive::OMPD_target:
+    case llvm::omp::Directive::OMPD_target_loop:
+    case llvm::omp::Directive::OMPD_target_parallel_do:
+    case llvm::omp::Directive::OMPD_target_parallel_do_simd:
+    case llvm::omp::Directive::OMPD_target_parallel_loop:
+    case llvm::omp::Directive::OMPD_target_teams_distribute:
+    case llvm::omp::Directive::OMPD_target_teams_distribute_parallel_do:
+    case llvm::omp::Directive::OMPD_target_teams_distribute_parallel_do_simd:
+    case llvm::omp::Directive::OMPD_target_teams_distribute_simd:
+    case llvm::omp::Directive::OMPD_target_teams_loop:
+    case llvm::omp::Directive::OMPD_target_simd:
       return true;
+      break;
+    default:
+      return false;
+      break;
+    }
   }
   return false;
 }
@@ -57,12 +73,12 @@ DataSharingProcessor::DataSharingProcessor(
     lower::AbstractConverter &converter, semantics::SemanticsContext &semaCtx,
     const List<Clause> &clauses, lower::pft::Evaluation &eval,
     bool shouldCollectPreDeterminedSymbols, bool useDelayedPrivatization,
-    lower::SymMap &symTable, bool isTargetPrivatization)
+    lower::SymMap &symTable, bool isTargetPrivitization)
     : converter(converter), semaCtx(semaCtx),
       firOpBuilder(converter.getFirOpBuilder()), clauses(clauses), eval(eval),
       shouldCollectPreDeterminedSymbols(shouldCollectPreDeterminedSymbols),
       useDelayedPrivatization(useDelayedPrivatization), symTable(symTable),
-      isTargetPrivatization(isTargetPrivatization), visitor(semaCtx) {
+      isTargetPrivitization(isTargetPrivitization), visitor(semaCtx) {
   eval.visit([&](const auto &functionParserNode) {
     parser::Walk(functionParserNode, visitor);
   });
@@ -73,11 +89,11 @@ DataSharingProcessor::DataSharingProcessor(lower::AbstractConverter &converter,
                                            lower::pft::Evaluation &eval,
                                            bool useDelayedPrivatization,
                                            lower::SymMap &symTable,
-                                           bool isTargetPrivatization)
+                                           bool isTargetPrivitization)
     : DataSharingProcessor(converter, semaCtx, {}, eval,
                            /*shouldCollectPreDeterminedSymols=*/false,
                            useDelayedPrivatization, symTable,
-                           isTargetPrivatization) {}
+                           isTargetPrivitization) {}
 
 void DataSharingProcessor::processStep1() {
   collectSymbolsForPrivatization();
@@ -549,7 +565,7 @@ void DataSharingProcessor::collectSymbols(
       // and not be added/captured by later directives. Parallel regions
       // will likely want the same captures to be shared and for SIMD it's
       // illegal to have firstprivate clauses.
-      if (isConstructWithTopLevelTarget(eval) && !isTargetPrivatization &&
+      if (isConstructWithTopLevelTarget(eval) && !isTargetPrivitization &&
           sym->test(semantics::Symbol::Flag::OmpFirstPrivate)) {
         return false;
       }

flang/lib/Lower/OpenMP/DataSharingProcessor.h

@@ -93,7 +93,7 @@ class DataSharingProcessor {
   bool useDelayedPrivatization;
   llvm::SmallSet<const semantics::Symbol *, 16> mightHaveReadHostSym;
   lower::SymMap &symTable;
-  bool isTargetPrivatization;
+  bool isTargetPrivitization;
   OMPConstructSymbolVisitor visitor;
   bool privatizationDone = false;
 
@@ -133,13 +133,13 @@ class DataSharingProcessor {
                        lower::pft::Evaluation &eval,
                        bool shouldCollectPreDeterminedSymbols,
                        bool useDelayedPrivatization, lower::SymMap &symTable,
-                       bool isTargetPrivatization = false);
+                       bool isTargetPrivitization = false);
 
   DataSharingProcessor(lower::AbstractConverter &converter,
                        semantics::SemanticsContext &semaCtx,
                        lower::pft::Evaluation &eval,
                        bool useDelayedPrivatization, lower::SymMap &symTable,
-                       bool isTargetPrivatization = false);
+                       bool isTargetPrivitization = false);
 
   // Privatisation is split into 3 steps:
   //

flang/lib/Lower/OpenMP/OpenMP.cpp

@@ -2390,6 +2390,9 @@ static bool isDuplicateMappedSymbol(
   concatSyms.append(mappedSyms.begin(), mappedSyms.end());
 
   auto checkSymbol = [&](const semantics::Symbol &checkSym) {
+    if (llvm::is_contained(concatSyms, &checkSym))
+      return true;
+
     return std::any_of(concatSyms.begin(), concatSyms.end(),
                        [&](auto v) { return v->GetUltimate() == checkSym; });
   };
@@ -2432,7 +2435,8 @@ genTargetOp(lower::AbstractConverter &converter, lower::SymMap &symTable,
                            lower::omp::isLastItemInQueue(item, queue),
                            /*useDelayedPrivatization=*/true, symTable,
                            /*isTargetPrivitization=*/true);
-  dsp.processStep1(&clauseOps);
+  dsp.processStep1();
+  dsp.processStep2(&clauseOps);
 
   // 5.8.1 Implicit Data-Mapping Attribute Rules
   // The following code follows the implicit data-mapping rules to map all the

flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp

@@ -58,7 +58,7 @@ class MapsForPrivatizedSymbolsPass
     // 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 canPassByValue = [&](mlir::Type type) {
+    auto isLiteralType = [&](mlir::Type type) {
       const mlir::DataLayout &dl = builder.getDataLayout();
       mlir::Type ptrTy = mlir::LLVM::LLVMPointerType::get(builder.getContext());
       uint64_t ptrSize = dl.getTypeSize(ptrTy);
@@ -112,7 +112,7 @@ class MapsForPrivatizedSymbolsPass
         mlir::omp::VariableCaptureKind::ByRef;
     if (fir::isa_trivial(fir::unwrapRefType(varPtr.getType())) ||
         fir::isa_char(fir::unwrapRefType(varPtr.getType()))) {
-      if (canPassByValue(fir::unwrapRefType(varPtr.getType()))) {
+      if (isLiteralType(fir::unwrapRefType(varPtr.getType()))) {
         captureKind = mlir::omp::VariableCaptureKind::ByCopy;
       }
     }

flang/lib/Semantics/resolve-directives.cpp

@@ -823,10 +823,11 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
       Symbol::Flag::OmpLastPrivate, Symbol::Flag::OmpReduction,
       Symbol::Flag::OmpLinear};
 
-  Symbol::Flags dataMappingAttributeFlags{Symbol::Flag::OmpMapTo,
-      Symbol::Flag::OmpMapFrom, Symbol::Flag::OmpMapToFrom,
-      Symbol::Flag::OmpMapStorage, Symbol::Flag::OmpMapDelete,
-      Symbol::Flag::OmpIsDevicePtr, Symbol::Flag::OmpHasDeviceAddr};
+  Symbol::Flags dataMappingAttributeFlags {
+      Symbol::Flag::OmpMapTo, Symbol::Flag::OmpMapFrom,
+      Symbol::Flag::OmpMapToFrom, Symbol::Flag::OmpMapStorage,
+      Symbol::Flag::OmpMapDelete, Symbol::Flag::OmpIsDevicePtr,
+      Symbol::Flag::OmpHasDeviceAddr};
 
   Symbol::Flags privateDataSharingAttributeFlags{Symbol::Flag::OmpPrivate,
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate};
@@ -2417,20 +2418,17 @@ static bool IsTargetCaptureImplicitlyFirstprivatizeable(const Symbol &symbol,
     std::map<parser::OmpVariableCategory::Value,
         parser::OmpDefaultmapClause::ImplicitBehavior>
         defaultMap) {
-  // If a Defaultmap clause is present for the current target scope, and it has
-  // specified behaviour other than Firstprivate for scalars then we exit early,
-  // as it overrides the implicit Firstprivatization of scalars OpenMP rule.
   if (!defaultMap.empty()) {
     if (llvm::is_contained(
-            defaultMap, parser::OmpVariableCategory::Value::All) &&
-        defaultMap[parser::OmpVariableCategory::Value::All] !=
+            defaultMap, parser::OmpVariableCategory::Value::Scalar) &&
+        defaultMap[parser::OmpVariableCategory::Value::Scalar] !=
             parser::OmpDefaultmapClause::ImplicitBehavior::Firstprivate) {
       return false;
     }
 
     if (llvm::is_contained(
-            defaultMap, parser::OmpVariableCategory::Value::Scalar) &&
-        defaultMap[parser::OmpVariableCategory::Value::Scalar] !=
+            defaultMap, parser::OmpVariableCategory::Value::All) &&
+        defaultMap[parser::OmpVariableCategory::Value::All] !=
             parser::OmpDefaultmapClause::ImplicitBehavior::Firstprivate) {
       return false;
     }
@@ -2443,7 +2441,7 @@ static bool IsTargetCaptureImplicitlyFirstprivatizeable(const Symbol &symbol,
     // TODO: Relax restriction as we progress privitization and further
     // investigate the flags we can intermix with.
     if (!(dsa & (dataSharingAttributeFlags | dataMappingAttributeFlags))
-            .none() ||
+             .none() ||
         !checkSym.flags().none() || semantics::IsAssumedShape(checkSym) ||
         semantics::IsAllocatableOrPointer(checkSym)) {
       return false;
@@ -2566,7 +2564,7 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
 
     bool taskGenDir = llvm::omp::taskGeneratingSet.test(dirContext.directive);
     bool targetDir = llvm::omp::allTargetSet.test(dirContext.directive);
-    bool parallelDir = llvm::omp::topParallelSet.test(dirContext.directive);
+    bool parallelDir = llvm::omp::allParallelSet.test(dirContext.directive);
     bool teamsDir = llvm::omp::allTeamsSet.test(dirContext.directive);
     bool isStaticStorageDuration = IsSymbolStaticStorageDuration(*symbol);
 
@@ -2626,7 +2624,7 @@ void OmpAttributeVisitor::CreateImplicitSymbols(const Symbol *symbol) {
       // 4) not mapped target variable  -> firstprivate
       //    - i.e. implicit, but meets OpenMP specification rules for
       //    firstprivate "promotion"
-      if (enableDelayedPrivatizationStaging &&
+      if (/*enableDelayedPrivatizationStaging && */
           IsTargetCaptureImplicitlyFirstprivatizeable(*symbol, prevDSA,
               dataSharingAttributeFlags, dataMappingAttributeFlags,
               dirContext.defaultMap)) {

flang/test/Integration/OpenMP/map-types-and-sizes.f90

@@ -6,17 +6,14 @@
 ! added to this directory and sub-directories.
 !===----------------------------------------------------------------------===!
 
-!RUN: %flang_fc1 -emit-llvm -fopenmp -mmlir --enable-delayed-privatization-staging=false -fopenmp-version=51 -fopenmp-targets=amdgcn-amd-amdhsa %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-NO-FPRIV
-!RUN: %flang_fc1 -emit-llvm -fopenmp -mmlir --enable-delayed-privatization-staging=true -fopenmp-version=51 -fopenmp-targets=amdgcn-amd-amdhsa %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-FPRIV
-
+!RUN: %flang_fc1 -emit-llvm -fopenmp -fopenmp-version=51 -fopenmp-targets=amdgcn-amd-amdhsa %s -o - | FileCheck %s
 
 !===============================================================================
 ! Check MapTypes for target implicit captures
 !===============================================================================
 
 !CHECK: @.offload_sizes = private unnamed_addr constant [1 x i64] [i64 4]
-!CHECK-FPRIV: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 289]
-!CHECK-NO-FPRIV: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 800]
+!CHECK: @.offload_maptypes = private unnamed_addr constant [1 x i64] [i64 289]
 subroutine mapType_scalar
   integer :: a
   !$omp target
@@ -375,8 +372,7 @@ subroutine mapType_nested_derived_type_member_idx()
 end subroutine
 
 !CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [2 x i64] [i64 8, i64 4]
-!CHECK-FPRIV: @.offload_maptypes{{.*}} = private unnamed_addr constant [2 x i64] [i64 544, i64 289]
-!CHECK-NO-FPRIV: @.offload_maptypes{{.*}} = private unnamed_addr constant [2 x i64] [i64 544, i64 800]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [2 x i64] [i64 544, i64 289]
 subroutine mapType_c_ptr
   use iso_c_binding, only : c_ptr, c_loc
   type(c_ptr) :: a
@@ -387,8 +383,7 @@ subroutine mapType_c_ptr
 end subroutine mapType_c_ptr
 
 !CHECK: @.offload_sizes{{.*}} = private unnamed_addr constant [1 x i64] [i64 1]
-!CHECK-FPRIV: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 289]
-!CHECK-NO-FPRIV: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 800]
+!CHECK: @.offload_maptypes{{.*}} = private unnamed_addr constant [1 x i64] [i64 289]
 subroutine mapType_char
   character :: a
   !$omp target

flang/test/Lower/OpenMP/DelayedPrivatization/target-private-implicit-scalar-map.f90

@@ -37,10 +37,6 @@ end subroutine target_allocatable
 
 ! Test the privatizer for `character`
 !
-! CHECK: omp.private {type = firstprivate}
-! CHECK-SAME: @[[FIRSTPRIVATE_SCALAR_SYM:[^[:space:]]+mapped_var[^[:space:]]+]]
-! CHECK-SAME:   : [[FIRSTPRIVATE_TYPE:i32]] copy {
-
 ! CHECK:      omp.private {type = private}
 ! CHECK-SAME:   @[[CHAR_PRIVATIZER_SYM:[^[:space:]]+char_var[^[:space:]]+]]
 ! CHECK-SAME:   : [[CHAR_TYPE:!fir.boxchar<1>]] init {
@@ -82,7 +78,6 @@ end subroutine target_allocatable
 
 ! Test the privatizer for `real(:)`'s lower bound
 !
-
 ! CHECK:      omp.private {type = private}
 ! CHECK-SAME:   @[[LB_PRIVATIZER_SYM:[^[:space:]]+lb[^[:space:]]+]]
 ! CHECK-SAME:   : [[LB_TYPE:i64]]{{$}}
@@ -145,29 +140,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:       %[[REAL_ARR_DESC_MAP]] -> %[[MAPPED_ARG2:[^,]+]]
 ! CHECK-SAME:       %[[CHAR_VAR_DESC_MAP]] -> %[[MAPPED_ARG3:.[^,]+]]
-! 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>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>, !fir.ref<!fir.boxchar<1>>
+! 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:     private(
-! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=0],
+! CHECK-SAME:       @[[ALLOC_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[ALLOC_ARG:[^,]+]] [map_idx=1],
 ! 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=1],
+! CHECK-SAME:       @[[ARR_PRIVATIZER_SYM]] %{{[^[:space:]]+}} -> %[[ARR_ARG:[^,]+]] [map_idx=2],
 ! CHECK-SAME:       @[[COMP_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[COMP_ARG:[^,]+]],
-! CHECK-SAME:       @[[CHAR_PRIVATIZER_SYM]] %{{[^[:space:]]+}}#0 -> %[[CHAR_ARG:[^,]+]] [map_idx=2]
-! CHECK-SAME:       @[[FIRSTPRIVATE_SCALAR_SYM]] %{{[^[:space:]]+}}#0 -> %[[FP_SCALAR_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>, !fir.ref<i32>)
+! 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-NOT:      fir.alloca
 ! CHECK:          hlfir.declare %[[ALLOC_ARG]]
 ! CHECK:          hlfir.declare %[[REAL_ARG]]