[flang][OpenACC] use correct type when create private box init recipe

[rscottmanley]

The recipe for initializing private box types was incorrect because hlfir::createTempFromMold() is not a suitable utility function when the box element type is a trivial type.

[llvmbot]

@llvm/pr-subscribers-openacc

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

Author: Scott Manley (rscottmanley)

Changes

The recipe for initializing private box types was incorrect because hlfir::createTempFromMold() is not a suitable utility function when the box element type is a trivial type.

---

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

2 Files Affected:

• (modified) flang/lib/Lower/OpenACC.cpp (+16-8)
• (modified) flang/test/Lower/OpenACC/acc-private.f90 (+30)

diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 3dd35ed9ae481..c83e277b996f3 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -522,13 +522,17 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
                                      mlir::Type ty, mlir::Location loc) {
   mlir::Value retVal = recipe.getInitRegion().front().getArgument(0);
   ty = fir::unwrapRefType(ty);
-  if (fir::isa_trivial(ty)) {
+
+  auto getDeclareOpForType = [&](mlir::Type ty) -> hlfir::DeclareOp {
     auto alloca = builder.create<fir::AllocaOp>(loc, ty);
-    auto declareOp = builder.create<hlfir::DeclareOp>(
+    return builder.create<hlfir::DeclareOp>(
         loc, alloca, accPrivateInitName, /*shape=*/nullptr,
         llvm::ArrayRef<mlir::Value>{}, /*dummy_scope=*/nullptr,
         fir::FortranVariableFlagsAttr{});
-    retVal = declareOp.getBase();
+  };
+
+  if (fir::isa_trivial(ty)) {
+    retVal = getDeclareOpForType(ty).getBase();
   } else if (auto seqTy = mlir::dyn_cast_or_null<fir::SequenceType>(ty)) {
     if (fir::isa_trivial(seqTy.getEleTy())) {
       mlir::Value shape;
@@ -552,12 +556,16 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
     }
   } else if (auto boxTy = mlir::dyn_cast_or_null<fir::BaseBoxType>(ty)) {
     mlir::Type innerTy = fir::unwrapRefType(boxTy.getEleTy());
-    if (!fir::isa_trivial(innerTy) && !mlir::isa<fir::SequenceType>(innerTy))
+    if (fir::isa_trivial(innerTy)) {
+      retVal = getDeclareOpForType(ty).getBase();
+    } else if (mlir::isa<fir::SequenceType>(innerTy)) {
+      fir::FirOpBuilder firBuilder{builder, recipe.getOperation()};
+      hlfir::Entity source = hlfir::Entity{retVal};
+      auto [temp, cleanup] = hlfir::createTempFromMold(loc, firBuilder, source);
+      retVal = temp;
+    } else {
       TODO(loc, "Unsupported boxed type in OpenACC privatization");
-    fir::FirOpBuilder firBuilder{builder, recipe.getOperation()};
-    hlfir::Entity source = hlfir::Entity{retVal};
-    auto [temp, cleanup] = hlfir::createTempFromMold(loc, firBuilder, source);
-    retVal = temp;
+    }
   }
   builder.create<mlir::acc::YieldOp>(loc, retVal);
 }
diff --git a/flang/test/Lower/OpenACC/acc-private.f90 b/flang/test/Lower/OpenACC/acc-private.f90
index 50c7a258bb567..356bb9d825d8e 100644
--- a/flang/test/Lower/OpenACC/acc-private.f90
+++ b/flang/test/Lower/OpenACC/acc-private.f90
@@ -87,6 +87,13 @@
 ! CHECK:   acc.yield %[[DECLARE]]#0 : !fir.box<!fir.array<?xi32>>
 ! CHECK: }
 
+! CHECK-LABEL: @privatization_ref_box_heap_i32 : !fir.ref<!fir.box<!fir.heap<i32>>> init {
+! CHECK: ^bb0(%arg0: !fir.ref<!fir.box<!fir.heap<i32>>>):
+! CHECK:   %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.heap<i32>>
+! CHECK:   %[[DECLARE:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "acc.private.init"} : (!fir.ref<!fir.box<!fir.heap<i32>>>) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
+! CHECK:   acc.yield %[[DECLARE]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>
+! CHECK: }
+
 ! CHECK-LABEL: acc.private.recipe @privatization_ref_box_heap_Uxi32 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>> init {
 ! CHECK: ^bb0(%[[ARG0:.*]]: !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>):
 ! CHECK:   %[[LOADBOX:.*]] = fir.load %[[ARG0]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>
@@ -292,6 +299,29 @@ subroutine acc_private_allocatable_array(a, n)
 ! CHECK: acc.loop {{.*}} private({{.*}}@privatization_ref_box_heap_Uxi32 -> %[[PRIVATE]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
 ! CHECK: acc.serial private(@privatization_ref_box_heap_Uxi32 -> %{{.*}} : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
 
+subroutine acc_private_allocatable_scalar(b, a, n)
+  integer :: a(n)
+  integer, allocatable :: b
+  integer :: i, n
+
+  !$acc parallel loop private(b)
+  do i = 1, n
+    a(i) = b
+  end do
+
+  !$acc serial private(b)
+  a(i) = b
+  !$acc end serial
+end subroutine
+
+! CHECK-LABEL: func.func @_QPacc_private_allocatable_scalar(
+! CHECK-SAME: %[[ARG0:.*]]: !fir.ref<!fir.box<!fir.heap<i32>>> {fir.bindc_name = "b"}
+! CHECK: %[[DECLA_B:.*]]:2 = hlfir.declare %arg0 dummy_scope %0 {fortran_attrs = #fir.var_attrs<allocatable>, uniq_name = "_QFacc_private_allocatable_scalarEb"} : (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.dscope) -> (!fir.ref<!fir.box<!fir.heap<i32>>>, !fir.ref<!fir.box<!fir.heap<i32>>>)
+! CHECK: acc.parallel {{.*}} {
+! CHECK: %[[PRIVATE:.*]] = acc.private varPtr(%[[DECLA_B]]#0 : !fir.ref<!fir.box<!fir.heap<i32>>>) -> !fir.ref<!fir.box<!fir.heap<i32>>> {name = "b"}
+! CHECK: acc.loop {{.*}} private({{.*}}@privatization_ref_box_heap_i32 -> %[[PRIVATE]] : !fir.ref<!fir.box<!fir.heap<i32>>>)
+! CHECK: acc.serial private(@privatization_ref_box_heap_i32 -> %{{.*}} : !fir.ref<!fir.box<!fir.heap<i32>>>) {
+
 subroutine acc_private_pointer_array(a, n)
   integer, pointer :: a(:)
   integer :: i, n

[razvanlupusoru]

It looks better than before :) I am approving because of that - I think it is bad that types were inconsistent before! Thank you!

It is still not quite complete though - as it allows for an uninitialized box to be created - and I do not think this is quite what one desires. In the case of allocatable - a check for allocation should be made - then an allocation of temp along with a fir.embox operation should be created.