From 43f05fb9c59194897bf75b66b8dfb0bab49ad409 Mon Sep 17 00:00:00 2001 From: Kajetan Puchalski Date: Fri, 13 Jun 2025 14:31:38 +0000 Subject: [PATCH 1/3] [flang][OpenMP] Support using copyprivate with fir.boxchar arguments Implement the lowering for passing a fir.boxchar argument to the copyprivate clause. Signed-off-by: Kajetan Puchalski --- flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 42 +++++++++++++++++----- flang/test/Lower/OpenMP/copyprivate5.f90 | 32 +++++++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) create mode 100644 flang/test/Lower/OpenMP/copyprivate5.f90 diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index 88baad8827e92..bf91623936cf1 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -727,12 +727,15 @@ class TypeInfo { // Is the type inside a box? bool isBox() const { return inBox; } + bool isBoxChar() const { return inBoxChar; } + private: void typeScan(mlir::Type type); std::optional charLen; llvm::SmallVector shape; bool inBox = false; + bool inBoxChar = false; }; void TypeInfo::typeScan(mlir::Type ty) { @@ -748,6 +751,9 @@ void TypeInfo::typeScan(mlir::Type ty) { typeScan(cty.getEleTy()); } else if (auto cty = mlir::dyn_cast(ty)) { charLen = cty.getLen(); + } else if (auto cty = mlir::dyn_cast(ty)) { + inBoxChar = true; + typeScan(cty.getEleTy()); } else if (auto hty = mlir::dyn_cast(ty)) { typeScan(hty.getEleTy()); } else if (auto pty = mlir::dyn_cast(ty)) { @@ -791,12 +797,6 @@ createCopyFunc(mlir::Location loc, lower::AbstractConverter &converter, fir::FortranVariableFlagsAttr attrs; if (varAttrs != fir::FortranVariableFlagsEnum::None) attrs = fir::FortranVariableFlagsAttr::get(builder.getContext(), varAttrs); - llvm::SmallVector typeparams; - if (typeInfo.getCharLength().has_value()) { - mlir::Value charLen = builder.createIntegerConstant( - loc, builder.getCharacterLengthType(), *typeInfo.getCharLength()); - typeparams.push_back(charLen); - } mlir::Value shape; if (!typeInfo.isBox() && !typeInfo.getShape().empty()) { llvm::SmallVector extents; @@ -805,11 +805,34 @@ createCopyFunc(mlir::Location loc, lower::AbstractConverter &converter, builder.createIntegerConstant(loc, builder.getIndexType(), extent)); shape = builder.create(loc, extents); } + mlir::Value dst = funcOp.getArgument(0); + mlir::Value src = funcOp.getArgument(1); + llvm::SmallVector typeparams; + if (typeInfo.isBoxChar()) { + // fir.boxchar will be passed here as fir.ref + auto loadDst = builder.create(loc, dst); + auto loadSrc = builder.create(loc, src); + // get the actual fir.ref type + mlir::Type refType = + fir::ReferenceType::get(mlir::cast(eleTy).getEleTy()); + auto unboxedDst = builder.create( + loc, refType, builder.getIndexType(), loadDst); + auto unboxedSrc = builder.create( + loc, refType, builder.getIndexType(), loadSrc); + // Add length to type parameters + typeparams.push_back(unboxedDst.getResult(1)); + dst = unboxedDst.getResult(0); + src = unboxedSrc.getResult(0); + } else if (typeInfo.getCharLength().has_value()) { + mlir::Value charLen = builder.createIntegerConstant( + loc, builder.getCharacterLengthType(), *typeInfo.getCharLength()); + typeparams.push_back(charLen); + } auto declDst = builder.create( - loc, funcOp.getArgument(0), copyFuncName + "_dst", shape, typeparams, + loc, dst, copyFuncName + "_dst", shape, typeparams, /*dummy_scope=*/nullptr, attrs); auto declSrc = builder.create( - loc, funcOp.getArgument(1), copyFuncName + "_src", shape, typeparams, + loc, src, copyFuncName + "_src", shape, typeparams, /*dummy_scope=*/nullptr, attrs); converter.copyVar(loc, declDst.getBase(), declSrc.getBase(), varAttrs); builder.create(loc); @@ -838,7 +861,8 @@ bool ClauseProcessor::processCopyprivate( // In these cases to retrieve the appropriate !fir.ref> to // access the data we need we must perform an alloca and then store to it // and retrieve the data from the new alloca. - if (mlir::isa(symType)) { + if (mlir::isa(symType) || + mlir::isa(symType)) { fir::FirOpBuilder &builder = converter.getFirOpBuilder(); auto alloca = builder.create(currentLocation, symType); builder.create(currentLocation, symVal, alloca); diff --git a/flang/test/Lower/OpenMP/copyprivate5.f90 b/flang/test/Lower/OpenMP/copyprivate5.f90 new file mode 100644 index 0000000000000..59509f82da343 --- /dev/null +++ b/flang/test/Lower/OpenMP/copyprivate5.f90 @@ -0,0 +1,32 @@ +! Test lowering of COPYPRIVATE with character arguments +! RUN: %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s + +! Testcase from: https://github.com/llvm/llvm-project/issues/142123 + +! CHECK-LABEL: func.func private @_copy_boxchar_c8xU( +! CHECK-SAME: %arg0: [[TYPE:!fir.ref>]], +! CHECK-SAME: %arg1: [[TYPE]]) attributes {llvm.linkage = #llvm.linkage} { +! CHECK: %[[RDST:.*]] = fir.load %arg0 : [[TYPE]] +! CHECK: %[[RSRC:.*]] = fir.load %arg1 : [[TYPE]] +! CHECK: %[[UDST:.*]]:2 = fir.unboxchar %[[RDST:.*]] : ([[UTYPE:!fir.boxchar<1>]]) -> ([[RTYPE:!fir.ref>]], [[ITYPE:index]]) +! CHECK: %[[USRC:.*]]:2 = fir.unboxchar %[[RSRC:.*]] : ([[UTYPE]]) -> ([[RTYPE]], [[ITYPE]]) +! CHECK: %[[DST:.*]]:2 = hlfir.declare %[[UDST:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "_copy_boxchar_c8xU_dst"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) +! CHECK: %[[SRC:.*]]:2 = hlfir.declare %[[USRC:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "_copy_boxchar_c8xU_src"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) +! CHECK: hlfir.assign %[[SRC:.*]]#0 to %[[DST:.*]]#0 : [[UTYPE]], [[UTYPE]] +! CHECK: return +! CHECK: } + +! CHECK-LABEL: func.func @_QPs(%arg0: !fir.boxchar<1> {fir.bindc_name = "c"}) { +! CHECK: omp.single copyprivate(%3 -> @_copy_boxchar_c8xU : !fir.ref>) { +! CHECK: omp.terminator +! CHECK: } + +subroutine s(c) +character(*) :: c +!$omp single copyprivate(c) +!$omp end single +end subroutine + +character(len=3) :: c +call s(c) +end From 16753d9e98d3a2332551773e069e8aaf809c177f Mon Sep 17 00:00:00 2001 From: Kajetan Puchalski Date: Mon, 16 Jun 2025 12:11:11 +0000 Subject: [PATCH 2/3] Expand test --- flang/test/Lower/OpenMP/copyprivate5.f90 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/flang/test/Lower/OpenMP/copyprivate5.f90 b/flang/test/Lower/OpenMP/copyprivate5.f90 index 59509f82da343..80d46e04ad3ef 100644 --- a/flang/test/Lower/OpenMP/copyprivate5.f90 +++ b/flang/test/Lower/OpenMP/copyprivate5.f90 @@ -17,13 +17,17 @@ ! CHECK: } ! CHECK-LABEL: func.func @_QPs(%arg0: !fir.boxchar<1> {fir.bindc_name = "c"}) { -! CHECK: omp.single copyprivate(%3 -> @_copy_boxchar_c8xU : !fir.ref>) { +! CHECK: %[[ALLOC:.*]] = fir.alloca !fir.boxchar<1> +! CHECK: fir.store %[[SRC:.*]] to %[[ALLOC:.*]] : !fir.ref> +! CHECK: omp.single copyprivate([[ALLOC:.*]] -> @_copy_boxchar_c8xU : !fir.ref>) { +! CHECK: hlfir.assign %[[NEW_VAL:.*]] to %[[SRC:.*]] : !fir.ref>, !fir.boxchar<1> ! CHECK: omp.terminator ! CHECK: } subroutine s(c) character(*) :: c !$omp single copyprivate(c) +c = "bar" !$omp end single end subroutine From 5b40c4111ea08e5fbf1ed83966f5c9b18b8fa6e2 Mon Sep 17 00:00:00 2001 From: Kajetan Puchalski Date: Tue, 17 Jun 2025 12:02:02 +0000 Subject: [PATCH 3/3] Update comment and test --- flang/lib/Lower/OpenMP/ClauseProcessor.cpp | 8 +++++--- flang/test/Lower/OpenMP/copyprivate5.f90 | 4 ++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp index bf91623936cf1..e98277b971265 100644 --- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp +++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp @@ -858,9 +858,11 @@ bool ClauseProcessor::processCopyprivate( // CopyPrivate variables must be passed by reference. However, in the case // of assumed shapes/vla the type is not a !fir.ref, but a !fir.box. - // In these cases to retrieve the appropriate !fir.ref> to - // access the data we need we must perform an alloca and then store to it - // and retrieve the data from the new alloca. + // In the case of character types, the passed in type can also be + // !fir.boxchar. In these cases to retrieve the appropriate + // !fir.ref> or !fir.ref> to access the data + // we need we must perform an alloca and then store to it and retrieve the + // data from the new alloca. if (mlir::isa(symType) || mlir::isa(symType)) { fir::FirOpBuilder &builder = converter.getFirOpBuilder(); diff --git a/flang/test/Lower/OpenMP/copyprivate5.f90 b/flang/test/Lower/OpenMP/copyprivate5.f90 index 80d46e04ad3ef..c75eb82a45e9f 100644 --- a/flang/test/Lower/OpenMP/copyprivate5.f90 +++ b/flang/test/Lower/OpenMP/copyprivate5.f90 @@ -10,8 +10,8 @@ ! CHECK: %[[RSRC:.*]] = fir.load %arg1 : [[TYPE]] ! CHECK: %[[UDST:.*]]:2 = fir.unboxchar %[[RDST:.*]] : ([[UTYPE:!fir.boxchar<1>]]) -> ([[RTYPE:!fir.ref>]], [[ITYPE:index]]) ! CHECK: %[[USRC:.*]]:2 = fir.unboxchar %[[RSRC:.*]] : ([[UTYPE]]) -> ([[RTYPE]], [[ITYPE]]) -! CHECK: %[[DST:.*]]:2 = hlfir.declare %[[UDST:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "_copy_boxchar_c8xU_dst"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) -! CHECK: %[[SRC:.*]]:2 = hlfir.declare %[[USRC:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "_copy_boxchar_c8xU_src"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) +! CHECK: %[[DST:.*]]:2 = hlfir.declare %[[UDST:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "[[NAME1:.*]]"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) +! CHECK: %[[SRC:.*]]:2 = hlfir.declare %[[USRC:.*]]#0 typeparams %[[UDST:.*]]#1 {uniq_name = "[[NAME2:.*]]"} : ([[RTYPE]], [[ITYPE]]) -> ([[UTYPE]], [[RTYPE]]) ! CHECK: hlfir.assign %[[SRC:.*]]#0 to %[[DST:.*]]#0 : [[UTYPE]], [[UTYPE]] ! CHECK: return ! CHECK: }