From a7ffe1cf4b96b07dec85dd435bbbef20a294bf85 Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Mon, 10 Nov 2025 18:42:40 -0600 Subject: [PATCH 1/2] [Flang][OpenMP] - Fix the mapping flags used on descriptors mapped by MapsForPrivatizedSymbolsPass The descriptors of a variable that has been privatized should be mapped `tofrom` instead of `to`. --- .../OpenMP/MapsForPrivatizedSymbols.cpp | 26 +++++++++---- .../target-private-allocatable.f90 | 2 +- .../Lower/OpenMP/optional-argument-map-2.f90 | 2 +- .../omp-maps-for-privatized-symbols.fir | 37 +++++++------------ 4 files changed, 34 insertions(+), 33 deletions(-) diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp index 0972861b8450a..4018d646609d0 100644 --- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp +++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp @@ -104,21 +104,33 @@ class MapsForPrivatizedSymbolsPass llvm::SmallVector boundsOps; if (needsBoundsOps(varPtr)) genBoundsOps(builder, varPtr, boundsOps); + mlir::Type varType = varPtr.getType(); mlir::omp::VariableCaptureKind captureKind = 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 (fir::isa_trivial(fir::unwrapRefType(varType)) || + fir::isa_char(fir::unwrapRefType(varType))) { + if (canPassByValue(fir::unwrapRefType(varType))) { captureKind = mlir::omp::VariableCaptureKind::ByCopy; } } + // TODO: We should be looking at the defaultmap here. However, at this time + // defaultmap on a target directive ins't modelled in the omp MLIR directive + // Once that is in place, we should move both lowering and this pass + // to use a utility function (like getImplicitMapTypeAndKind in + // lib/Lower/OpenMP/OpenMP.cpp) to get the mapFlag value here. + mlir::omp::ClauseMapFlags mapFlag; + if (fir::isa_trivial(fir::unwrapRefType(varType)) || + fir::isa_char(fir::unwrapRefType(varType))) + mapFlag = mlir::omp::ClauseMapFlags::to; + else + mapFlag = mlir::omp::ClauseMapFlags::to | mlir::omp::ClauseMapFlags::from; return omp::MapInfoOp::create( - builder, loc, varPtr.getType(), varPtr, - TypeAttr::get(llvm::cast(varPtr.getType()) - .getElementType()), - builder.getAttr(omp::ClauseMapFlags::to), + builder, loc, varType, varPtr, + TypeAttr::get( + llvm::cast(varType).getElementType()), + builder.getAttr(mapFlag), builder.getAttr(captureKind), /*varPtrPtr=*/Value{}, /*members=*/SmallVector{}, diff --git a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-allocatable.f90 b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-allocatable.f90 index 272f34fc0fd1a..cfe42367b051b 100644 --- a/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-allocatable.f90 +++ b/flang/test/Lower/OpenMP/DelayedPrivatization/target-private-allocatable.f90 @@ -72,7 +72,7 @@ end subroutine target_allocatable ! CPU-SAME: {bindc_name = "alloc_var", {{.*}}} ! CPU: %[[VAR_DECL:.*]]:2 = hlfir.declare %[[VAR_ALLOC]] ! CPU: %[[BASE_ADDR:.*]] = fir.box_offset %[[VAR_DECL]]#0 base_addr : (!fir.ref>>) -> [[MEMBER_TYPE:.*]] -! CPU: %[[MEMBER:.*]] = omp.map.info var_ptr(%[[VAR_DECL]]#0 : [[TYPE]], i32) map_clauses(to) capture(ByRef) var_ptr_ptr(%[[BASE_ADDR]] : [[MEMBER_TYPE:.*]]) -> {{.*}} +! CPU: %[[MEMBER:.*]] = omp.map.info var_ptr(%[[VAR_DECL]]#0 : [[TYPE]], i32) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[BASE_ADDR]] : [[MEMBER_TYPE:.*]]) -> {{.*}} ! CPU: %[[MAP_VAR:.*]] = omp.map.info var_ptr(%[[VAR_DECL]]#0 : [[TYPE]], [[DESC_TYPE]]) map_clauses(to) capture(ByRef) members(%[[MEMBER]] : [0] : !fir.llvm_ptr>) -> !fir.ref>> ! CPU: omp.target map_entries(%[[MAP_VAR]] -> %arg0, %[[MEMBER]] -> %arg1 : [[TYPE]], [[MEMBER_TYPE]]) private( diff --git a/flang/test/Lower/OpenMP/optional-argument-map-2.f90 b/flang/test/Lower/OpenMP/optional-argument-map-2.f90 index a787ad78dfafd..5d64b95d13ad6 100644 --- a/flang/test/Lower/OpenMP/optional-argument-map-2.f90 +++ b/flang/test/Lower/OpenMP/optional-argument-map-2.f90 @@ -72,7 +72,7 @@ end module mod ! CHECK-FPRIV: %[[VAL_13:.*]] = arith.subi %[[VAL_12]]#1, %[[VAL_11]] : index ! CHECK-FPRIV: %[[VAL_14:.*]] = omp.map.bounds lower_bound(%[[VAL_10]] : index) upper_bound(%[[VAL_13]] : index) extent(%[[VAL_12]]#1 : index) stride(%[[VAL_11]] : index) start_idx(%[[VAL_10]] : index) {stride_in_bytes = true} ! CHECK-FPRIV: %[[VAL_16:.*]] = fir.box_offset %[[VAL_0]] base_addr : (!fir.ref>) -> !fir.llvm_ptr>> -! CHECK-FPRIV: %[[VAL_17:.*]] = omp.map.info var_ptr(%[[VAL_0]] : !fir.ref>, !fir.char<1,?>) map_clauses(to) capture(ByRef) var_ptr_ptr(%[[VAL_16]] : !fir.llvm_ptr>>) bounds(%[[VAL_14]]) -> !fir.llvm_ptr>> {name = ""} +! CHECK-FPRIV: %[[VAL_17:.*]] = omp.map.info var_ptr(%[[VAL_0]] : !fir.ref>, !fir.char<1,?>) map_clauses(tofrom) capture(ByRef) var_ptr_ptr(%[[VAL_16]] : !fir.llvm_ptr>>) bounds(%[[VAL_14]]) -> !fir.llvm_ptr>> {name = ""} ! CHECK-FPRIV: %[[VAL_18:.*]] = omp.map.info var_ptr(%[[VAL_0]] : !fir.ref>, !fir.boxchar<1>) map_clauses(to) capture(ByRef) members(%[[VAL_17]] : [0] : !fir.llvm_ptr>>) -> !fir.ref> ! CHECK-FPRIV: omp.target map_entries(%[[VAL_7]] -> %[[VAL_19:.*]], %[[VAL_18]] -> %[[VAL_20:.*]], %[[VAL_17]] -> %[[VAL_21:.*]] : !fir.ref>, !fir.ref>, !fir.llvm_ptr>>) private(@_QMmodFroutine_boxcharEa_firstprivate_boxchar_c8xU %[[VAL_3]]#0 -> %[[VAL_22:.*]] [map_idx=1] : !fir.boxchar<1>) { ! CHECK-FPRIV: %[[VAL_23:.*]] = arith.constant 4 : index diff --git a/flang/test/Transforms/omp-maps-for-privatized-symbols.fir b/flang/test/Transforms/omp-maps-for-privatized-symbols.fir index 10a76126ed054..6054c70a2700d 100644 --- a/flang/test/Transforms/omp-maps-for-privatized-symbols.fir +++ b/flang/test/Transforms/omp-maps-for-privatized-symbols.fir @@ -6,7 +6,12 @@ module attributes {omp.is_target_device = false} { // extract box address, see if it is null, etc omp.yield(%arg1: !fir.ref>>) } - + omp.private {type = firstprivate} @_QFtarget_simpleEfp_int_firstprivate_i32 : i32 copy { + ^bb0(%arg0: !fir.ref, %arg1: !fir.ref): + %0 = fir.load %arg0 : !fir.ref + hlfir.assign %0 to %arg1 : i32, !fir.ref + omp.yield(%arg1 : !fir.ref) + } func.func @_QPtarget_simple() { %0 = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFtarget_simpleEa"} %1:2 = hlfir.declare %0 {uniq_name = "_QFtarget_simpleEa"} : (!fir.ref) -> (!fir.ref, !fir.ref) @@ -15,34 +20,18 @@ module attributes {omp.is_target_device = false} { %4 = fir.embox %3 : (!fir.heap) -> !fir.box> fir.store %4 to %2 : !fir.ref>> %5:2 = hlfir.declare %2 {fortran_attrs = #fir.var_attrs, uniq_name = "_QFtarget_simpleEsimple_var"} : (!fir.ref>>) -> (!fir.ref>>, !fir.ref>>) + %6 = fir.alloca i32 {bindc_name = "fp_int", uniq_name = "_QFtarget_simpleEfp_int"} + %7:2 = hlfir.declare %6 {uniq_name = "_QFtarget_simpleEfp_int"} : (!fir.ref) -> (!fir.ref, !fir.ref) %c2_i32 = arith.constant 2 : i32 hlfir.assign %c2_i32 to %1#0 : i32, !fir.ref - %6 = omp.map.info var_ptr(%1#1 : !fir.ref, i32) map_clauses(to) capture(ByRef) -> !fir.ref {name = "a"} - omp.target map_entries(%6 -> %arg0 : !fir.ref) private(@_QFtarget_simpleEsimple_var_private_ref_box_heap_i32 %5#0 -> %arg1 : !fir.ref>>) { - %11:2 = hlfir.declare %arg0 {uniq_name = "_QFtarget_simpleEa"} : (!fir.ref) -> (!fir.ref, !fir.ref) - %12:2 = hlfir.declare %arg1 {fortran_attrs = #fir.var_attrs, uniq_name = "_QFtarget_simpleEsimple_var"} : (!fir.ref>>) -> (!fir.ref>>, !fir.ref>>) - %c10_i32 = arith.constant 10 : i32 - %13 = fir.load %11#0 : !fir.ref - %14 = arith.addi %c10_i32, %13 : i32 - hlfir.assign %14 to %12#0 realloc : i32, !fir.ref>> + %8 = omp.map.info var_ptr(%1#1 : !fir.ref, i32) map_clauses(to) capture(ByRef) -> !fir.ref {name = "a"} + omp.target map_entries(%8 -> %arg0 : !fir.ref) private(@_QFtarget_simpleEsimple_var_private_ref_box_heap_i32 %5#0 -> %arg1, @_QFtarget_simpleEfp_int_firstprivate_i32 %7#0 -> %arg2 : !fir.ref>>, !fir.ref) { omp.terminator } - %7 = fir.load %5#1 : !fir.ref>> - %8 = fir.box_addr %7 : (!fir.box>) -> !fir.heap - %9 = fir.convert %8 : (!fir.heap) -> i64 - %c0_i64 = arith.constant 0 : i64 - %10 = arith.cmpi ne, %9, %c0_i64 : i64 - fir.if %10 { - %11 = fir.load %5#1 : !fir.ref>> - %12 = fir.box_addr %11 : (!fir.box>) -> !fir.heap - fir.freemem %12 : !fir.heap - %13 = fir.zero_bits !fir.heap - %14 = fir.embox %13 : (!fir.heap) -> !fir.box> - fir.store %14 to %5#1 : !fir.ref>> - } return } } // CHECK: %[[MAP0:.*]] = omp.map.info var_ptr({{.*}} : !fir.ref, i32) map_clauses(to) capture(ByRef) -> !fir.ref {name = "a"} -// CHECK: %[[MAP1:.*]] = omp.map.info var_ptr({{.*}} : !fir.ref>>, !fir.box>) map_clauses(to) capture(ByRef) -> !fir.ref>> -// CHECK: omp.target map_entries(%[[MAP0]] -> %arg0, %[[MAP1]] -> %arg1 : !fir.ref, !fir.ref>>) +// CHECK: %[[MAP1:.*]] = omp.map.info var_ptr({{.*}} : !fir.ref>>, !fir.box>) map_clauses(tofrom) capture(ByRef) -> !fir.ref>> +// CHECK: %[[MAP2:.*]] = omp.map.info var_ptr({{.*}} : !fir.ref, i32) map_clauses(to) capture(ByCopy) -> !fir.ref +// CHECK: omp.target map_entries(%[[MAP0]] -> %arg0, %[[MAP1]] -> %arg1, %[[MAP2]] -> %arg2 : !fir.ref, !fir.ref>>, !fir.ref) From 97c8a851656cd3f2801515ee5710abe537b4af49 Mon Sep 17 00:00:00 2001 From: Pranav Bhandarkar Date: Tue, 11 Nov 2025 11:45:49 -0600 Subject: [PATCH 2/2] update comment --- flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp index 4018d646609d0..6404e1892ca5d 100644 --- a/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp +++ b/flang/lib/Optimizer/OpenMP/MapsForPrivatizedSymbols.cpp @@ -114,11 +114,9 @@ class MapsForPrivatizedSymbolsPass captureKind = mlir::omp::VariableCaptureKind::ByCopy; } } - // TODO: We should be looking at the defaultmap here. However, at this time - // defaultmap on a target directive ins't modelled in the omp MLIR directive - // Once that is in place, we should move both lowering and this pass - // to use a utility function (like getImplicitMapTypeAndKind in - // lib/Lower/OpenMP/OpenMP.cpp) to get the mapFlag value here. + + // Use tofrom if what we are mapping is not a trivial type. In all + // likelihood, it is a descriptor mlir::omp::ClauseMapFlags mapFlag; if (fir::isa_trivial(fir::unwrapRefType(varType)) || fir::isa_char(fir::unwrapRefType(varType)))