[flang][acc] Fix issue with privatization recipe for box ref #137869
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Razvan Lupusoru (razvanlupusoru) ChangesWhen privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested. Full diff: https://github.com/llvm/llvm-project/pull/137869.diff 2 Files Affected:
diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 06697b2dbc1d5..82daa05c165cb 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -831,9 +831,9 @@ fir::ShapeOp genShapeOp(mlir::OpBuilder &builder, fir::SequenceType seqTy,
template <typename RecipeOp>
static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
- mlir::Type ty, mlir::Location loc) {
+ mlir::Type argTy, mlir::Location loc) {
mlir::Value retVal = recipe.getInitRegion().front().getArgument(0);
- ty = fir::unwrapRefType(ty);
+ mlir::Type unwrappedTy = fir::unwrapRefType(argTy);
auto getDeclareOpForType = [&](mlir::Type ty) -> hlfir::DeclareOp {
auto alloca = builder.create<fir::AllocaOp>(loc, ty);
@@ -843,9 +843,10 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
fir::FortranVariableFlagsAttr{});
};
- 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(unwrappedTy)) {
+ retVal = getDeclareOpForType(unwrappedTy).getBase();
+ } else if (auto seqTy =
+ mlir::dyn_cast_or_null<fir::SequenceType>(unwrappedTy)) {
if (fir::isa_trivial(seqTy.getEleTy())) {
mlir::Value shape;
llvm::SmallVector<mlir::Value> extents;
@@ -866,15 +867,33 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
/*dummy_scope=*/nullptr, fir::FortranVariableFlagsAttr{});
retVal = declareOp.getBase();
}
- } else if (auto boxTy = mlir::dyn_cast_or_null<fir::BaseBoxType>(ty)) {
+ } else if (auto boxTy =
+ mlir::dyn_cast_or_null<fir::BaseBoxType>(unwrappedTy)) {
mlir::Type innerTy = fir::unwrapRefType(boxTy.getEleTy());
if (fir::isa_trivial(innerTy)) {
- retVal = getDeclareOpForType(ty).getBase();
+ retVal = getDeclareOpForType(unwrappedTy).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;
+ if (fir::isa_ref_type(argTy)) {
+ // When the temp is created - it is not a reference - thus we can
+ // end up with a type inconsistency. Therefore ensure storage is created
+ // for it.
+ retVal = getDeclareOpForType(unwrappedTy).getBase();
+ mlir::Value storeDst = retVal;
+ if (fir::unwrapRefType(retVal.getType()) != temp.getType()) {
+ // `createTempFromMold` makes the unfortunate choice to lose the
+ // `fir.heap` and `fir.ptr` types when wrapping with a box. Namely,
+ // when wrapping a `fir.heap<fir.array>`, it will create instead a
+ // `fir.box<fir.array>`. Cast here to deal with this inconsistency.
+ storeDst = firBuilder.createConvert(
+ loc, firBuilder.getRefType(temp.getType()), retVal);
+ }
+ builder.create<fir::StoreOp>(loc, temp, storeDst);
+ } else {
+ retVal = temp;
+ }
} else {
TODO(loc, "Unsupported boxed type in OpenACC privatization");
}
diff --git a/flang/test/Lower/OpenACC/acc-private.f90 b/flang/test/Lower/OpenACC/acc-private.f90
index 76e23b8915e92..b1bfb02439f03 100644
--- a/flang/test/Lower/OpenACC/acc-private.f90
+++ b/flang/test/Lower/OpenACC/acc-private.f90
@@ -84,7 +84,11 @@
! CHECK: %[[SHAPE:.*]] = fir.shape %[[BOX_DIMS]]#1 : (index) -> !fir.shape<1>
! CHECK: %[[TEMP:.*]] = fir.allocmem !fir.array<?xi32>, %[[BOX_DIMS]]#1 {bindc_name = ".tmp", uniq_name = ""}
! CHECK: %[[DECLARE:.*]]:2 = hlfir.declare %[[TEMP]](%[[SHAPE]]) {uniq_name = ".tmp"} : (!fir.heap<!fir.array<?xi32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xi32>>, !fir.heap<!fir.array<?xi32>>)
-! CHECK: acc.yield %[[DECLARE]]#0 : !fir.box<!fir.array<?xi32>>
+! CHECK: %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.ptr<!fir.array<?xi32>>>
+! CHECK: %[[DECLAREBOX:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "acc.private.init"} : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>)
+! CHECK: %[[CONV:.*]] = fir.convert %[[DECLAREBOX]]#0 : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: fir.store %[[DECLARE]]#0 to %[[CONV]] : !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: acc.yield %[[DECLAREBOX]]#0 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>
! CHECK: }
! CHECK-LABEL: @privatization_ref_box_heap_i32 : !fir.ref<!fir.box<!fir.heap<i32>>> init {
@@ -102,7 +106,11 @@
! CHECK: %[[SHAPE:.*]] = fir.shape %[[BOX_DIMS]]#1 : (index) -> !fir.shape<1>
! CHECK: %[[TEMP:.*]] = fir.allocmem !fir.array<?xi32>, %[[BOX_DIMS]]#1 {bindc_name = ".tmp", uniq_name = ""}
! CHECK: %[[DECLARE:.*]]:2 = hlfir.declare %[[TEMP]](%[[SHAPE]]) {uniq_name = ".tmp"} : (!fir.heap<!fir.array<?xi32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xi32>>, !fir.heap<!fir.array<?xi32>>)
-! CHECK: acc.yield %[[DECLARE]]#0 : !fir.box<!fir.array<?xi32>>
+! CHECK: %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>>
+! CHECK: %[[DECLAREBOX:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "acc.private.init"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+! CHECK: %[[CONV:.*]] = fir.convert %[[DECLAREBOX]]#0 : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) -> !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: fir.store %[[DECLARE]]#0 to %[[CONV]] : !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: acc.yield %[[DECLAREBOX]]#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>
! CHECK: }
! CHECK-LABEL: acc.private.recipe @privatization_box_Uxi32 : !fir.box<!fir.array<?xi32>> init {
|
|
@llvm/pr-subscribers-openacc Author: Razvan Lupusoru (razvanlupusoru) ChangesWhen privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested. Full diff: https://github.com/llvm/llvm-project/pull/137869.diff 2 Files Affected:
diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 06697b2dbc1d5..82daa05c165cb 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -831,9 +831,9 @@ fir::ShapeOp genShapeOp(mlir::OpBuilder &builder, fir::SequenceType seqTy,
template <typename RecipeOp>
static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
- mlir::Type ty, mlir::Location loc) {
+ mlir::Type argTy, mlir::Location loc) {
mlir::Value retVal = recipe.getInitRegion().front().getArgument(0);
- ty = fir::unwrapRefType(ty);
+ mlir::Type unwrappedTy = fir::unwrapRefType(argTy);
auto getDeclareOpForType = [&](mlir::Type ty) -> hlfir::DeclareOp {
auto alloca = builder.create<fir::AllocaOp>(loc, ty);
@@ -843,9 +843,10 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
fir::FortranVariableFlagsAttr{});
};
- 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(unwrappedTy)) {
+ retVal = getDeclareOpForType(unwrappedTy).getBase();
+ } else if (auto seqTy =
+ mlir::dyn_cast_or_null<fir::SequenceType>(unwrappedTy)) {
if (fir::isa_trivial(seqTy.getEleTy())) {
mlir::Value shape;
llvm::SmallVector<mlir::Value> extents;
@@ -866,15 +867,33 @@ static void genPrivateLikeInitRegion(mlir::OpBuilder &builder, RecipeOp recipe,
/*dummy_scope=*/nullptr, fir::FortranVariableFlagsAttr{});
retVal = declareOp.getBase();
}
- } else if (auto boxTy = mlir::dyn_cast_or_null<fir::BaseBoxType>(ty)) {
+ } else if (auto boxTy =
+ mlir::dyn_cast_or_null<fir::BaseBoxType>(unwrappedTy)) {
mlir::Type innerTy = fir::unwrapRefType(boxTy.getEleTy());
if (fir::isa_trivial(innerTy)) {
- retVal = getDeclareOpForType(ty).getBase();
+ retVal = getDeclareOpForType(unwrappedTy).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;
+ if (fir::isa_ref_type(argTy)) {
+ // When the temp is created - it is not a reference - thus we can
+ // end up with a type inconsistency. Therefore ensure storage is created
+ // for it.
+ retVal = getDeclareOpForType(unwrappedTy).getBase();
+ mlir::Value storeDst = retVal;
+ if (fir::unwrapRefType(retVal.getType()) != temp.getType()) {
+ // `createTempFromMold` makes the unfortunate choice to lose the
+ // `fir.heap` and `fir.ptr` types when wrapping with a box. Namely,
+ // when wrapping a `fir.heap<fir.array>`, it will create instead a
+ // `fir.box<fir.array>`. Cast here to deal with this inconsistency.
+ storeDst = firBuilder.createConvert(
+ loc, firBuilder.getRefType(temp.getType()), retVal);
+ }
+ builder.create<fir::StoreOp>(loc, temp, storeDst);
+ } else {
+ retVal = temp;
+ }
} else {
TODO(loc, "Unsupported boxed type in OpenACC privatization");
}
diff --git a/flang/test/Lower/OpenACC/acc-private.f90 b/flang/test/Lower/OpenACC/acc-private.f90
index 76e23b8915e92..b1bfb02439f03 100644
--- a/flang/test/Lower/OpenACC/acc-private.f90
+++ b/flang/test/Lower/OpenACC/acc-private.f90
@@ -84,7 +84,11 @@
! CHECK: %[[SHAPE:.*]] = fir.shape %[[BOX_DIMS]]#1 : (index) -> !fir.shape<1>
! CHECK: %[[TEMP:.*]] = fir.allocmem !fir.array<?xi32>, %[[BOX_DIMS]]#1 {bindc_name = ".tmp", uniq_name = ""}
! CHECK: %[[DECLARE:.*]]:2 = hlfir.declare %[[TEMP]](%[[SHAPE]]) {uniq_name = ".tmp"} : (!fir.heap<!fir.array<?xi32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xi32>>, !fir.heap<!fir.array<?xi32>>)
-! CHECK: acc.yield %[[DECLARE]]#0 : !fir.box<!fir.array<?xi32>>
+! CHECK: %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.ptr<!fir.array<?xi32>>>
+! CHECK: %[[DECLAREBOX:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "acc.private.init"} : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>)
+! CHECK: %[[CONV:.*]] = fir.convert %[[DECLAREBOX]]#0 : (!fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>) -> !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: fir.store %[[DECLARE]]#0 to %[[CONV]] : !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: acc.yield %[[DECLAREBOX]]#0 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>
! CHECK: }
! CHECK-LABEL: @privatization_ref_box_heap_i32 : !fir.ref<!fir.box<!fir.heap<i32>>> init {
@@ -102,7 +106,11 @@
! CHECK: %[[SHAPE:.*]] = fir.shape %[[BOX_DIMS]]#1 : (index) -> !fir.shape<1>
! CHECK: %[[TEMP:.*]] = fir.allocmem !fir.array<?xi32>, %[[BOX_DIMS]]#1 {bindc_name = ".tmp", uniq_name = ""}
! CHECK: %[[DECLARE:.*]]:2 = hlfir.declare %[[TEMP]](%[[SHAPE]]) {uniq_name = ".tmp"} : (!fir.heap<!fir.array<?xi32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xi32>>, !fir.heap<!fir.array<?xi32>>)
-! CHECK: acc.yield %[[DECLARE]]#0 : !fir.box<!fir.array<?xi32>>
+! CHECK: %[[ALLOCA:.*]] = fir.alloca !fir.box<!fir.heap<!fir.array<?xi32>>>
+! CHECK: %[[DECLAREBOX:.*]]:2 = hlfir.declare %[[ALLOCA]] {uniq_name = "acc.private.init"} : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) -> (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>, !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>)
+! CHECK: %[[CONV:.*]] = fir.convert %[[DECLAREBOX]]#0 : (!fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>) -> !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: fir.store %[[DECLARE]]#0 to %[[CONV]] : !fir.ref<!fir.box<!fir.array<?xi32>>>
+! CHECK: acc.yield %[[DECLAREBOX]]#0 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xi32>>>>
! CHECK: }
! CHECK-LABEL: acc.private.recipe @privatization_box_Uxi32 : !fir.box<!fir.array<?xi32>> init {
|
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…7869) When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…7869) When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…7869) When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…7869) When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
…7869) When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe. The updated logic now creates storage when a box ref is requested.
rscottmanley
pushed a commit
to rscottmanley/llvm-project
that referenced
this pull request
May 20, 2025
Between firstprivate, private and reduction init regions, the difference is largely whether or not the temp that is created is initialized or not. Some recent fixes were made to privatization (llvm#135698, llvm#137869) but did not get propagated to reductions, even though they essentially do the same thing. To mitigate this descrepency in the future, refactor the init region recipes so they can be shared between the three recipe ops. Also add "none" to the OpenACC_ReductionOperator enum for better error checking.
rscottmanley
pushed a commit
to rscottmanley/llvm-project
that referenced
this pull request
May 20, 2025
Between firstprivate, private and reduction init regions, the difference is largely whether or not the temp that is created is initialized or not. Some recent fixes were made to privatization (llvm#135698, llvm#137869) but did not get propagated to reductions, even though they essentially do the same thing. To mitigate this descrepency in the future, refactor the init region recipes so they can be shared between the three recipe ops. Also add "none" to the OpenACC_ReductionOperator enum for better error checking.
rscottmanley
added a commit
that referenced
this pull request
May 20, 2025
Between firstprivate, private and reduction init regions, the difference is largely whether or not the temp that is created is initialized or not. Some recent fixes were made to privatization (#135698, #137869) but did not get propagated to reductions, even though they need to return the yield the same things from their init regions. To mitigate this discrepancy in the future, refactor the init region recipes so they can be shared between the three recipe ops. Also add "none" to the OpenACC_ReductionOperator enum for better error checking.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…140652) Between firstprivate, private and reduction init regions, the difference is largely whether or not the temp that is created is initialized or not. Some recent fixes were made to privatization (llvm#135698, llvm#137869) but did not get propagated to reductions, even though they need to return the yield the same things from their init regions. To mitigate this discrepancy in the future, refactor the init region recipes so they can be shared between the three recipe ops. Also add "none" to the OpenACC_ReductionOperator enum for better error checking.
ajaden-codes
pushed a commit
to Jaddyen/llvm-project
that referenced
this pull request
Jun 6, 2025
…140652) Between firstprivate, private and reduction init regions, the difference is largely whether or not the temp that is created is initialized or not. Some recent fixes were made to privatization (llvm#135698, llvm#137869) but did not get propagated to reductions, even though they need to return the yield the same things from their init regions. To mitigate this discrepancy in the future, refactor the init region recipes so they can be shared between the three recipe ops. Also add "none" to the OpenACC_ReductionOperator enum for better error checking.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When privatizing allocatable/pointer arrays, the code was creating a temporary but this was a box type. This led to inconsistency between the input and output of recipe.
The updated logic now creates storage when a box ref is requested.