[Flang][OpenMP] Increase restrictions on descriptor deferral (llvm#3381)

ronlieb · web-flow · commit b451b1fc9425 · 2025-08-01T21:04:40.000-04:00
diff --git a/flang/include/flang/Optimizer/OpenMP/Passes.td b/flang/include/flang/Optimizer/OpenMP/Passes.td
@@ -23,7 +23,7 @@ def MapInfoFinalizationPass
   let dependentDialects = ["mlir::omp::OpenMPDialect"];
 
   let options = [Option<"deferDescMapping", "opt-defer-desc-mapping",
-                      "bool", /*default=*/"false",
+                      "bool", /*default=*/"true",
                       "Activates or deactivates deferred descriptor mapping, "
                       "which delays mapping of top-level descriptors to target "
                       "regions and target data regions">];
diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
@@ -494,7 +494,7 @@ static void parseCodeGenArgs(Fortran::frontend::CodeGenOptions &opts,
 
   opts.DeferDescriptorMapping =
       args.hasFlag(clang::driver::options::OPT_fdefer_desc_map,
-                   clang::driver::options::OPT_fno_defer_desc_map, false);
+                   clang::driver::options::OPT_fno_defer_desc_map, true);
 
   if (const llvm::opt::Arg *arg =
           args.getLastArg(clang::driver::options::OPT_complex_range_EQ)) {
diff --git a/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp b/flang/lib/Optimizer/OpenMP/MapInfoFinalization.cpp
@@ -82,6 +82,10 @@ class MapInfoFinalizationPass
   ///      |                  |
   std::map<mlir::Operation *, mlir::Value> localBoxAllocas;
 
+  // List of deferrable descriptors to process at the end of
+  // the pass.
+  llvm::SmallVector<mlir::Operation *> deferrableDesc;
+
   /// getMemberUserList gathers all users of a particular MapInfoOp that are
   /// other MapInfoOp's and places them into the mapMemberUsers list, which
   /// records the map that the current argument MapInfoOp "op" is part of
@@ -126,20 +130,51 @@ class MapInfoFinalizationPass
                     });
   }
 
+  // Check if the declaration operation we have refers to a dummy
+  // function argument.
+  bool isDummyArgument(mlir::Operation *op) {
+    if (auto declareOp = mlir::dyn_cast<hlfir::DeclareOp>(op))
+      if (auto dummyScope = declareOp.getDummyScope())
+        return true;
+    return false;
+  }
+
+  // Relevant for OpenMP < 5.2, where attach semantics and rules don't exist.
+  // As descriptors were an unspoken implementation detail in these versions
+  // there's certain cases where the user (and the compiler implementation)
+  // can create data mapping errors by having temporary descriptors stuck
+  // in memory. To avoid this we can defer the descriptor mapping in these
+  // cases until target or target data regions, when we can be sure they
+  // have a clear limited scope on device.
+  bool canDeferDescriptorMapping(mlir::Value descriptor) {
+    if (!fir::isAllocatableType(descriptor.getType()) ||
+        !fir::isPointerType(descriptor.getType())) {
+      if (isDummyArgument(descriptor.getDefiningOp()) &&
+          (fir::isAssumedType(descriptor.getType()) ||
+           fir::isAssumedShape(descriptor.getType()))) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   /// When provided a MapInfoOp containing a descriptor type that
   /// we must expand into multiple maps this function will extract
   /// the value from it and return it, in certain cases we must
   /// generate a new allocation to store into so that the
   /// fir::BoxOffsetOp we utilise to access the descriptor datas
   /// base address can be utilised.
   mlir::Value getDescriptorFromBoxMap(mlir::omp::MapInfoOp boxMap,
-                                      fir::FirOpBuilder &builder) {
+                                      fir::FirOpBuilder &builder,
+                                      bool &canDescBeDeferred) {
     mlir::Value descriptor = boxMap.getVarPtr();
     if (!fir::isTypeWithDescriptor(boxMap.getVarType()))
       if (auto addrOp = mlir::dyn_cast_if_present<fir::BoxAddrOp>(
               boxMap.getVarPtr().getDefiningOp()))
         descriptor = addrOp.getVal();
 
+    canDescBeDeferred = canDeferDescriptorMapping(descriptor);
+
     if (!mlir::isa<fir::BaseBoxType>(descriptor.getType()) &&
         !fir::factory::isOptionalArgument(descriptor.getDefiningOp()))
       return descriptor;
@@ -291,7 +326,7 @@ class MapInfoFinalizationPass
     if (llvm::isa_and_nonnull<mlir::omp::TargetExitDataOp,
                               mlir::omp::TargetUpdateOp>(target)) {
       mapFlags flags = mapFlags(mapTypeFlag);
-       if (!IsHasDeviceAddr)
+      if (!IsHasDeviceAddr)
         flags |= mapFlags::OMP_MAP_DESCRIPTOR;
       return llvm::to_underlying(flags);
     }
@@ -415,7 +450,9 @@ class MapInfoFinalizationPass
 
     // TODO: map the addendum segment of the descriptor, similarly to the
     // base address/data pointer member.
-    mlir::Value descriptor = getDescriptorFromBoxMap(op, builder);
+    bool descCanBeDeferred = false;
+    mlir::Value descriptor =
+        getDescriptorFromBoxMap(op, builder, descCanBeDeferred);
 
     mlir::ArrayAttr newMembersAttr;
     mlir::SmallVector<mlir::Value> newMembers;
@@ -491,6 +528,10 @@ class MapInfoFinalizationPass
         /*partial_map=*/builder.getBoolAttr(false));
     op.replaceAllUsesWith(newDescParentMapOp.getResult());
     op->erase();
+
+    if (descCanBeDeferred)
+      deferrableDesc.push_back(newDescParentMapOp);
+
     return newDescParentMapOp;
   }
 
@@ -784,6 +825,7 @@ class MapInfoFinalizationPass
       // clear all local allocations we made for any boxes in any prior
       // iterations from previous function scopes.
       localBoxAllocas.clear();
+      deferrableDesc.clear();
 
       // First, walk `omp.map.info` ops to see if any of them have varPtrs
       // with an underlying type of fir.char<k, ?>, i.e a character
@@ -817,7 +859,7 @@ class MapInfoFinalizationPass
         op.getBoundsMutable().append(boundsOps);
       });
 
-      // Next, walk `omp.map.info` ops to see if any record members should be
+            // Next, walk `omp.map.info` ops to see if any record members should be
       // implicitly mapped.
       // TODO/FIXME/UPDATE: I believe we need to add implicit capture of
       // allocatable members of arbitrary depths for this before we can
@@ -1059,18 +1101,14 @@ class MapInfoFinalizationPass
       // data and the runtime should correctly associate the data with the
       // descriptor and bind together and allow clean mapping and execution.
       if (deferDescMapping) {
-        func->walk([&](mlir::omp::MapInfoOp op) {
-          if (fir::isTypeWithDescriptor(op.getVarType()) ||
-              mlir::isa_and_present<fir::BoxAddrOp>(
-                  op.getVarPtr().getDefiningOp())) {
-            mlir::Operation *targetUser = getFirstTargetUser(op);
-            assert(targetUser &&
-                   "expected user of map operation was not found");
-            builder.setInsertionPoint(op);
-            removeTopLevelDescriptor(op, builder, targetUser);
-            addImplictDescriptorMapToTargetDataOp(op, builder, targetUser);
-          }
-        });
+        for (auto *op : deferrableDesc) {
+          auto mapOp = llvm::dyn_cast<mlir::omp::MapInfoOp>(op);
+          mlir::Operation *targetUser = getFirstTargetUser(mapOp);
+          assert(targetUser && "expected user of map operation was not found");
+          builder.setInsertionPoint(mapOp);
+          removeTopLevelDescriptor(mapOp, builder, targetUser);
+          addImplictDescriptorMapToTargetDataOp(mapOp, builder, targetUser);
+        }
       }
 
       // Wait until after we have generated all of our maps to add them onto
diff --git a/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90 b/flang/test/Lower/OpenMP/use-device-ptr-to-use-device-addr.f90
@@ -1,12 +1,11 @@
 ! The "use_device_addr" was added to the "target data" directive in OpenMP 5.0.
-! RUN: %flang_fc1 -fdefer-desc-map -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
-! RUN: bbc -fdefer-desc-map -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
+! RUN: %flang_fc1 -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
+! RUN: bbc -emit-hlfir -fopenmp -fopenmp-version=50 %s -o - | FileCheck %s
 ! This tests primary goal is to check the promotion of non-CPTR arguments from
 ! use_device_ptr to use_device_addr works, without breaking any functionality.
 
 !CHECK: func.func @{{.*}}only_use_device_ptr()
-
-!CHECK: omp.target_data map_entries(%{{.*}}, %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) use_device_ptr(%{{.*}} -> %{{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
+!CHECK: omp.target_data use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) use_device_ptr(%{{.*}} -> %{{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
 subroutine only_use_device_ptr
     use iso_c_binding
     integer, pointer, dimension(:) :: array
@@ -18,7 +17,7 @@ subroutine only_use_device_ptr
 end subroutine
 
 !CHECK: func.func @{{.*}}mix_use_device_ptr_and_addr()
-!CHECK: omp.target_data map_entries(%{{.*}}, %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>) use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>) use_device_ptr({{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
+!CHECK: omp.target_data use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>) use_device_ptr({{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
 subroutine mix_use_device_ptr_and_addr
     use iso_c_binding
     integer, pointer, dimension(:) :: array
@@ -30,7 +29,7 @@ subroutine mix_use_device_ptr_and_addr
 end subroutine
 
 !CHECK: func.func @{{.*}}only_use_device_addr()
-!CHECK: omp.target_data map_entries(%{{.*}}, %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) {
+!CHECK: omp.target_data use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) {
 subroutine only_use_device_addr
     use iso_c_binding
     integer, pointer, dimension(:) :: array
@@ -42,7 +41,7 @@ subroutine only_use_device_addr
 end subroutine
 
 !CHECK: func.func @{{.*}}mix_use_device_ptr_and_addr_and_map()
-!CHECK: omp.target_data map_entries(%{{.*}}, %{{.*}}, %{{.*}}, %{{.*}} : !fir.ref<i32>, !fir.ref<i32>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>) use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>) use_device_ptr(%{{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
+!CHECK: omp.target_data map_entries(%{{.*}}, %{{.*}} : !fir.ref<i32>, !fir.ref<i32>) use_device_addr(%{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}}, %{{.*}} -> %{{.*}} : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xf32>>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>, !fir.llvm_ptr<!fir.ref<!fir.array<?xf32>>>) use_device_ptr(%{{.*}} : !fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>) {
 subroutine mix_use_device_ptr_and_addr_and_map
     use iso_c_binding
     integer :: i, j
diff --git a/flang/test/Lower/volatile-openmp.f90 b/flang/test/Lower/volatile-openmp.f90
@@ -1,4 +1,4 @@
-! RUN: bbc -fdefer-desc-map --strict-fir-volatile-verifier -fopenmp %s -o - | FileCheck %s
+! RUN: bbc --strict-fir-volatile-verifier -fopenmp %s -o - | FileCheck %s
 type t
 integer, pointer :: array(:)
 end type
@@ -46,8 +46,8 @@
 ! CHECK:           %[[VAL_34:.*]] = arith.subi %[[VAL_33]]#1, %[[VAL_1]] : index
 ! CHECK:           %[[VAL_35:.*]] = omp.map.bounds lower_bound(%[[VAL_0]] : index) upper_bound(%[[VAL_34]] : index) extent(%[[VAL_33]]#1 : index) stride(%[[VAL_33]]#2 : index) start_idx(%[[VAL_32]]#0 : index) {stride_in_bytes = true}
 ! CHECK:           %[[VAL_36:.*]] = fir.box_offset %[[VAL_10]]#1 base_addr : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>, volatile>) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>
-! CHECK:           %[[VAL_37:.*]] = fir.load %[[VAL_36]] : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>
-! CHECK:           %[[VAL_38:.*]] = omp.map.info var_ptr(%[[VAL_37]] : !fir.ref<!fir.array<?xi32>>, i32) map_clauses(descriptor_base_addr, to) capture(ByRef) bounds(%[[VAL_35]]) -> !fir.ref<!fir.array<?xi32>> {name = "array1"}
-! CHECK:           omp.target_enter_data map_entries(%[[VAL_38]] : !fir.ref<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_37:.*]] = omp.map.info var_ptr(%[[VAL_10]]#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>, volatile>, i32) map_clauses(descriptor_base_addr, to) capture(ByRef) var_ptr_ptr(%[[VAL_36]] : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) bounds(%[[VAL_35]]) -> !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>> {name = ""}
+! CHECK:           %[[VAL_38:.*]] = omp.map.info var_ptr(%[[VAL_10]]#1 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>, volatile>, !fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>) map_clauses(always, descriptor, to) capture(ByRef) members(%[[VAL_37]] : [0] : !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>) -> !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>, volatile> {name = "array1"}
+! CHECK:           omp.target_enter_data map_entries(%[[VAL_38]], %[[VAL_37]] : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>, volatile>, volatile>, !fir.llvm_ptr<!fir.ref<!fir.array<?xi32>>>)
 ! CHECK:           return
 ! CHECK:         }
diff --git a/flang/test/Transforms/omp-map-info-finalization.fir b/flang/test/Transforms/omp-map-info-finalization.fir
@@ -1,5 +1,5 @@
 // RUN: fir-opt  --split-input-file --omp-map-info-finalization %s | FileCheck %s
-// XFAIL: *
+
 func.func @test_descriptor_expansion_pass(%arg0: !fir.box<!fir.array<?xi32>>) {
   %0 = fir.alloca !fir.box<!fir.heap<i32>>
   %1 = fir.zero_bits !fir.heap<i32>
diff --git a/flang/tools/bbc/bbc.cpp b/flang/tools/bbc/bbc.cpp
@@ -146,7 +146,7 @@ static llvm::cl::opt<bool>
     deferDescMap("fdefer-desc-map",
                  llvm::cl::desc("disable or enable OpenMP deference of mapping "
                                 "for top-level descriptors"),
-                 llvm::cl::init(false));
+                 llvm::cl::init(true));
 
 static llvm::cl::opt<std::string> enableDoConcurrentToOpenMPConversion(
     "fdo-concurrent-to-openmp",
