[flang][openacc] Fixed bounds for full array sections. #70859
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
We have to use 0 lowerbound for data operands like `a(:)`.
llvmbot
added
flang
|
As I understand, this should affect OpenMP as well, but none of OpenMP LIT tests failed. |
|
@llvm/pr-subscribers-openacc @llvm/pr-subscribers-flang-fir-hlfir Author: Slava Zakharin (vzakhari) ChangesWe have to use 0 lowerbound for data operands like Full diff: https://github.com/llvm/llvm-project/pull/70859.diff 2 Files Affected:
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index 33b8198a2518be8..1e9d3e68b0b2baf 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -707,7 +707,10 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
asFortran << lexpr->AsFortran();
}
} else {
- lbound = defaultLb ? zero : baseLb;
+ // If the lower bound is not specified, then the section
+ // starts from offset 0 of the dimension.
+ // Note that the lowerbound in the BoundsOp is always 0-based.
+ lbound = zero;
}
asFortran << ':';
const auto &upper{std::get<1>(triplet->t)};
diff --git a/flang/test/Lower/OpenACC/acc-enter-data.f90 b/flang/test/Lower/OpenACC/acc-enter-data.f90
index 0938d137ca02c36..d83d10320ec273e 100644
--- a/flang/test/Lower/OpenACC/acc-enter-data.f90
+++ b/flang/test/Lower/OpenACC/acc-enter-data.f90
@@ -288,10 +288,11 @@ subroutine acc_enter_data_dummy(a, b, n, m)
!CHECK: acc.enter_data dataOperands(%[[CREATE1]] : !fir.ref<!fir.array<?xf32>>)
!$acc enter data create(b(:))
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!CHECK: %[[ONE:.*]] = arith.constant 1 : index
!CHECK: %[[LBEXT:.*]] = arith.addi %[[EXT_B]], %[[N_IDX]] : index
!CHECK: %[[UB:.*]] = arith.subi %[[LBEXT]], %[[ONE]] : index
-!CHECK: %[[BOUND1:.*]] = acc.bounds lowerbound(%[[N_IDX]] : index) upperbound(%[[UB]] : index) extent(%[[EXT_B]] : index) stride(%[[ONE]] : index) startIdx(%[[N_IDX]] : index)
+!CHECK: %[[BOUND1:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) extent(%[[EXT_B]] : index) stride(%[[ONE]] : index) startIdx(%[[N_IDX]] : index)
!FIR: %[[CREATE1:.*]] = acc.create varPtr(%[[B]] : !fir.ref<!fir.array<?xf32>>) bounds(%[[BOUND1]]) -> !fir.ref<!fir.array<?xf32>> {name = "b(:)", structured = false}
!HLFIR: %[[CREATE1:.*]] = acc.create varPtr(%[[DECLB]]#1 : !fir.ref<!fir.array<?xf32>>) bounds(%[[BOUND1]]) -> !fir.ref<!fir.array<?xf32>> {name = "b(:)", structured = false}
!CHECK: acc.enter_data dataOperands(%[[CREATE1]] : !fir.ref<!fir.array<?xf32>>)
@@ -323,19 +324,21 @@ subroutine acc_enter_data_non_default_lb()
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.ref<!fir.array<10xi32>>)
!$acc enter data create(a(:))
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!CHECK: %[[ONE:.*]] = arith.constant 1 : index
!CHECK: %[[LBEXT:.*]] = arith.addi %[[EXTENT_C10]], %[[BASELB]] : index
!CHECK: %[[UB:.*]] = arith.subi %[[LBEXT]], %[[ONE]] : index
-!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[BASELB]] : index) upperbound(%[[UB]] : index) extent(%[[EXTENT_C10]] : index) stride(%[[ONE]] : index) startIdx(%[[BASELB]] : index)
+!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) extent(%[[EXTENT_C10]] : index) stride(%[[ONE]] : index) startIdx(%[[BASELB]] : index)
!FIR: %[[CREATE:.*]] = acc.create varPtr(%[[A]] : !fir.ref<!fir.array<10xi32>>) bounds(%[[BOUND]]) -> !fir.ref<!fir.array<10xi32>> {name = "a(:)", structured = false}
!HLFIR: %[[CREATE:.*]] = acc.create varPtr(%[[DECLA]]#1 : !fir.ref<!fir.array<10xi32>>) bounds(%[[BOUND]]) -> !fir.ref<!fir.array<10xi32>> {name = "a(:)", structured = false}
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.ref<!fir.array<10xi32>>)
!$acc enter data create(a(:6))
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!CHECK: %[[ONE:.*]] = arith.constant 1 : index
!CHECK: %[[SECTIONUB:.*]] = arith.constant 6 : index
!CHECK: %[[UB:.*]] = arith.subi %[[SECTIONUB]], %[[BASELB]] : index
-!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[BASELB]] : index) upperbound(%[[UB]] : index) stride(%[[ONE]] : index) startIdx(%[[BASELB]] : index)
+!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) stride(%[[ONE]] : index) startIdx(%[[BASELB]] : index)
!FIR: %[[CREATE:.*]] = acc.create varPtr(%[[A]] : !fir.ref<!fir.array<10xi32>>) bounds(%[[BOUND]]) -> !fir.ref<!fir.array<10xi32>> {name = "a(:6)", structured = false}
!HLFIR: %[[CREATE:.*]] = acc.create varPtr(%[[DECLA]]#1 : !fir.ref<!fir.array<10xi32>>) bounds(%[[BOUND]]) -> !fir.ref<!fir.array<10xi32>> {name = "a(:6)", structured = false}
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.ref<!fir.array<10xi32>>)
@@ -503,6 +506,7 @@ subroutine acc_enter_data_assumed(a, b, n, m)
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.ref<!fir.array<?xf32>>)
!$acc enter data create(b(:m))
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!CHECK: %[[C0:.*]] = arith.constant 0 : index
!FIR: %[[DIMS0:.*]]:3 = fir.box_dims %[[B]], %[[C0]] : (!fir.box<!fir.array<?xf32>>, index) -> (index, index, index)
!HLFIR: %[[DIMS0:.*]]:3 = fir.box_dims %[[DECLB]]#1, %[[C0]] : (!fir.box<!fir.array<?xf32>>, index) -> (index, index, index)
@@ -510,7 +514,7 @@ subroutine acc_enter_data_assumed(a, b, n, m)
!HLFIR: %[[LOAD_M:.*]] = fir.load %[[DECLM]]#0 : !fir.ref<i32>
!CHECK: %[[CONVERT_M:.*]] = fir.convert %[[LOAD_M]] : (i32) -> index
!CHECK: %[[UB:.*]] = arith.subi %[[CONVERT_M]], %[[LB_C10_IDX]] : index
-!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[LB_C10_IDX]] : index) upperbound(%[[UB]] : index) stride(%[[DIMS0]]#2 : index) startIdx(%[[LB_C10_IDX]] : index) {strideInBytes = true}
+!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) stride(%[[DIMS0]]#2 : index) startIdx(%[[LB_C10_IDX]] : index) {strideInBytes = true}
!FIR: %[[BOX_ADDR:.*]] = fir.box_addr %[[B]] : (!fir.box<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>>
!HLFIR: %[[BOX_ADDR:.*]] = fir.box_addr %[[DECLB]]#1 : (!fir.box<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>>
!CHECK: %[[CREATE:.*]] = acc.create varPtr(%[[BOX_ADDR]] : !fir.ref<!fir.array<?xf32>>) bounds(%[[BOUND]]) -> !fir.ref<!fir.array<?xf32>> {name = "b(:m)", structured = false}
@@ -559,6 +563,7 @@ subroutine acc_enter_data_allocatable()
!$acc enter data create(a(:))
!FIR: %[[BOX_A_0:.*]] = fir.load %[[A]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
!HLFIR: %[[BOX_A_0:.*]] = fir.load %[[DECLA]]#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!CHECK: %[[ONE:.*]] = arith.constant 1 : index
!FIR: %[[BOX_A_1:.*]] = fir.load %[[A]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
!HLFIR: %[[BOX_A_1:.*]] = fir.load %[[DECLA]]#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
@@ -572,7 +577,7 @@ subroutine acc_enter_data_allocatable()
!CHECK: %[[DIMS2:.*]]:3 = fir.box_dims %[[BOX_A_2]], %[[C0]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>, index) -> (index, index, index)
!CHECK: %[[LBEXT:.*]] = arith.addi %[[DIMS2]]#1, %[[DIMS0]]#0 : index
!CHECK: %[[UB:.*]] = arith.subi %[[LBEXT]], %[[ONE]] : index
-!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[DIMS0]]#0 : index) upperbound(%[[UB:.*]] : index) stride(%[[DIMS1]]#2 : index) startIdx(%[[DIMS0]]#0 : index) {strideInBytes = true}
+!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB:.*]] : index) stride(%[[DIMS1]]#2 : index) startIdx(%[[DIMS0]]#0 : index) {strideInBytes = true}
!CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[BOX_A_0]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>) -> !fir.heap<!fir.array<?xf32>>
!CHECK: %[[CREATE:.*]] = acc.create varPtr(%[[BOX_ADDR]] : !fir.heap<!fir.array<?xf32>>) bounds(%[[BOUND]]) -> !fir.heap<!fir.array<?xf32>> {name = "a(:)", structured = false}
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.heap<!fir.array<?xf32>>)
@@ -621,6 +626,7 @@ subroutine acc_enter_data_allocatable()
!$acc enter data create(a(:7))
!FIR: %[[BOX_A_0:.*]] = fir.load %[[A]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
!HLFIR: %[[BOX_A_0:.*]] = fir.load %[[DECLA]]#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
+!CHECK: %[[ZERO:.*]] = arith.constant 0 : index
!FIR: %[[BOX_A_1:.*]] = fir.load %[[A]] : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
!HLFIR: %[[BOX_A_1:.*]] = fir.load %[[DECLA]]#1 : !fir.ref<!fir.box<!fir.heap<!fir.array<?xf32>>>>
!CHECK: %[[C0:.*]] = arith.constant 0 : index
@@ -629,7 +635,7 @@ subroutine acc_enter_data_allocatable()
!CHECK: %[[DIMS1:.*]]:3 = fir.box_dims %[[BOX_A_0]], %[[C0]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>, index) -> (index, index, index)
!CHECK: %[[C7:.*]] = arith.constant 7 : index
!CHECK: %[[UB:.*]] = arith.subi %[[C7]], %[[DIMS0]]#0 : index
-!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[DIMS0]]#0 : index) upperbound(%[[UB]] : index) stride(%[[DIMS1]]#2 : index) startIdx(%[[DIMS0]]#0 : index) {strideInBytes = true}
+!CHECK: %[[BOUND:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) stride(%[[DIMS1]]#2 : index) startIdx(%[[DIMS0]]#0 : index) {strideInBytes = true}
!CHECK: %[[BOX_ADDR:.*]] = fir.box_addr %[[BOX_A_0]] : (!fir.box<!fir.heap<!fir.array<?xf32>>>) -> !fir.heap<!fir.array<?xf32>>
!CHECK: %[[CREATE:.*]] = acc.create varPtr(%[[BOX_ADDR]] : !fir.heap<!fir.array<?xf32>>) bounds(%[[BOUND]]) -> !fir.heap<!fir.array<?xf32>> {name = "a(:7)", structured = false}
!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.heap<!fir.array<?xf32>>)
|
clementval
reviewed
Oct 31, 2023
| !CHECK: %[[ONE:.*]] = arith.constant 1 : index | ||
| !CHECK: %[[LBEXT:.*]] = arith.addi %[[EXT_B]], %[[N_IDX]] : index | ||
| !CHECK: %[[UB:.*]] = arith.subi %[[LBEXT]], %[[ONE]] : index | ||
| !CHECK: %[[BOUND1:.*]] = acc.bounds lowerbound(%[[N_IDX]] : index) upperbound(%[[UB]] : index) extent(%[[EXT_B]] : index) stride(%[[ONE]] : index) startIdx(%[[N_IDX]] : index) | ||
| !CHECK: %[[BOUND1:.*]] = acc.bounds lowerbound(%[[ZERO]] : index) upperbound(%[[UB]] : index) extent(%[[EXT_B]] : index) stride(%[[ONE]] : index) startIdx(%[[N_IDX]] : index) |
There was a problem hiding this comment.
If we set the lb to zero then the ub needs to be adjusted as well otherwise it doesn't match.
vzakhari
changed the title
It does, but I think OpenACC tests the bounds generation a little more strictly than the tests I've added so far for the bounds, which are primarily just some lit tests to make sure fortran generated the bounds as expected in certain cases and then using those bounds to test further lowering to LLVM-IR. However, if something was going to fail it would likely fail an openmp/libomptarget/offloading test e.g. https://github.com/llvm/llvm-project/blob/main/openmp/libomptarget/test/offloading/fortran/basic-target-region-3D-array-section.f90 but they're not really testable without an AMD GPU and I'm fairly certain none of the upstream buildbots run these tests at the moment so it won't fail a buildbot (nothing that compiles Flang and runs OpenMP tests with AMD GPU offload). I could test this PR tomorrow and if it breaks anything create another PR that can be approved and merged at the same time by you or incorporated into this patch. Or if we're in a bit of a hurry to land this, I can test they all work reasonably after this patch lands and create a fix if they do not. Whatever works best. |
|
Thank you for the reply, @agozillon! I will appreciated it if you could test the patch end-to-end! I can postpone the merge for a day, no problem. |
agozillon
approved these changes
Nov 1, 2023
There was a problem hiding this comment.
Works as expected, as does testing with another test case using the syntax a(:) that we don't currently cover! Thank you very much for your patience and the great fix. It all LGTM. If anything unexpected crops up that is OpenMP related just ping me and I am happy to look into it.
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.
We have to use 0-based lowerbound/upperbound for data operands like
a(:).