[flang][openacc] Generate data bounds for array addressing. #71254
Conversation
This file contains 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
In cases like `copy(array(N))` it is still useful to represent the data operand uniformly with `copy(array(N:N))`. This change generates data bounds even if it is not an array section with the triplets. The lower and the upper bounds are the same and the extent is one in this case. I did not enable this for OpenMP by default, because I am not sure if this is what the OpenMP devs want (though, I hope we can use the same).
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-fir-hlfir Author: Slava Zakharin (vzakhari) ChangesIn cases like Full diff: https://github.com/llvm/llvm-project/pull/71254.diff 3 Files Affected:
diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index 2ea4f53e94081f6..f4903f607a2da8c 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -660,7 +660,7 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
Fortran::lower::StatementContext &stmtCtx,
const std::list<Fortran::parser::SectionSubscript> &subscripts,
std::stringstream &asFortran, fir::ExtendedValue &dataExv,
- mlir::Value baseAddr) {
+ mlir::Value baseAddr, bool treatIndexAsSection = false) {
int dimension = 0;
mlir::Type idxTy = builder.getIndexType();
mlir::Type boundTy = builder.getType<BoundsType>();
@@ -669,8 +669,9 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
mlir::Value zero = builder.createIntegerConstant(loc, idxTy, 0);
mlir::Value one = builder.createIntegerConstant(loc, idxTy, 1);
for (const auto &subscript : subscripts) {
- if (const auto *triplet{
- std::get_if<Fortran::parser::SubscriptTriplet>(&subscript.u)}) {
+ const auto *triplet{
+ std::get_if<Fortran::parser::SubscriptTriplet>(&subscript.u)};
+ if (triplet || treatIndexAsSection) {
if (dimension != 0)
asFortran << ',';
mlir::Value lbound, ubound, extent;
@@ -689,9 +690,21 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
strideInBytes = true;
}
- const auto &lower{std::get<0>(triplet->t)};
+ const Fortran::lower::SomeExpr *lower{nullptr};
+ if (triplet) {
+ if (const auto &tripletLb{std::get<0>(triplet->t)})
+ lower = Fortran::semantics::GetExpr(*tripletLb);
+ } else {
+ const auto &index{std::get<Fortran::parser::IntExpr>(subscript.u)};
+ lower = Fortran::semantics::GetExpr(index);
+ if (lower->Rank() > 0) {
+ mlir::emitError(
+ loc, "vector subscript cannot be used for an array section");
+ break;
+ }
+ }
if (lower) {
- lval = Fortran::semantics::GetIntValue(lower);
+ lval = Fortran::evaluate::ToInt64(*lower);
if (lval) {
if (defaultLb) {
lbound = builder.createIntegerConstant(loc, idxTy, *lval - 1);
@@ -701,13 +714,11 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
}
asFortran << *lval;
} else {
- const Fortran::lower::SomeExpr *lexpr =
- Fortran::semantics::GetExpr(*lower);
mlir::Value lb =
- fir::getBase(converter.genExprValue(loc, *lexpr, stmtCtx));
+ fir::getBase(converter.genExprValue(loc, *lower, stmtCtx));
lb = builder.createConvert(loc, baseLb.getType(), lb);
lbound = builder.create<mlir::arith::SubIOp>(loc, lb, baseLb);
- asFortran << lexpr->AsFortran();
+ asFortran << lower->AsFortran();
}
} else {
// If the lower bound is not specified, then the section
@@ -715,45 +726,54 @@ genBoundsOps(fir::FirOpBuilder &builder, mlir::Location loc,
// Note that the lowerbound in the BoundsOp is always 0-based.
lbound = zero;
}
- asFortran << ':';
- const auto &upper{std::get<1>(triplet->t)};
- if (upper) {
- uval = Fortran::semantics::GetIntValue(upper);
- if (uval) {
- if (defaultLb) {
- ubound = builder.createIntegerConstant(loc, idxTy, *uval - 1);
+
+ if (!triplet) {
+ // If it is a scalar subscript, then the upper bound
+ // is equal to the lower bound, and the extent is one.
+ ubound = lbound;
+ extent = one;
+ } else {
+ asFortran << ':';
+ const auto &upper{std::get<1>(triplet->t)};
+
+ if (upper) {
+ uval = Fortran::semantics::GetIntValue(upper);
+ if (uval) {
+ if (defaultLb) {
+ ubound = builder.createIntegerConstant(loc, idxTy, *uval - 1);
+ } else {
+ mlir::Value ub = builder.createIntegerConstant(loc, idxTy, *uval);
+ ubound = builder.create<mlir::arith::SubIOp>(loc, ub, baseLb);
+ }
+ asFortran << *uval;
} else {
- mlir::Value ub = builder.createIntegerConstant(loc, idxTy, *uval);
+ const Fortran::lower::SomeExpr *uexpr =
+ Fortran::semantics::GetExpr(*upper);
+ mlir::Value ub =
+ fir::getBase(converter.genExprValue(loc, *uexpr, stmtCtx));
+ ub = builder.createConvert(loc, baseLb.getType(), ub);
ubound = builder.create<mlir::arith::SubIOp>(loc, ub, baseLb);
+ asFortran << uexpr->AsFortran();
}
- asFortran << *uval;
- } else {
- const Fortran::lower::SomeExpr *uexpr =
- Fortran::semantics::GetExpr(*upper);
- mlir::Value ub =
- fir::getBase(converter.genExprValue(loc, *uexpr, stmtCtx));
- ub = builder.createConvert(loc, baseLb.getType(), ub);
- ubound = builder.create<mlir::arith::SubIOp>(loc, ub, baseLb);
- asFortran << uexpr->AsFortran();
}
- }
- if (lower && upper) {
- if (lval && uval && *uval < *lval) {
- mlir::emitError(loc, "zero sized array section");
- break;
- } else if (std::get<2>(triplet->t)) {
- const auto &strideExpr{std::get<2>(triplet->t)};
- if (strideExpr) {
- mlir::emitError(loc, "stride cannot be specified on "
- "an array section");
+ if (lower && upper) {
+ if (lval && uval && *uval < *lval) {
+ mlir::emitError(loc, "zero sized array section");
break;
+ } else if (std::get<2>(triplet->t)) {
+ const auto &strideExpr{std::get<2>(triplet->t)};
+ if (strideExpr) {
+ mlir::emitError(loc, "stride cannot be specified on "
+ "an array section");
+ break;
+ }
}
}
- }
- if (!ubound) {
- // ub = extent - 1
- extent = fir::factory::readExtent(builder, loc, dataExv, dimension);
- ubound = builder.create<mlir::arith::SubIOp>(loc, extent, one);
+ if (!ubound) {
+ // ub = extent - 1
+ extent = fir::factory::readExtent(builder, loc, dataExv, dimension);
+ ubound = builder.create<mlir::arith::SubIOp>(loc, extent, one);
+ }
}
mlir::Value bound = builder.create<BoundsOp>(
loc, boundTy, lbound, ubound, extent, stride, strideInBytes, baseLb);
@@ -770,7 +790,7 @@ mlir::Value gatherDataOperandAddrAndBounds(
Fortran::semantics::SemanticsContext &semanticsContext,
Fortran::lower::StatementContext &stmtCtx, const ObjectType &object,
mlir::Location operandLocation, std::stringstream &asFortran,
- llvm::SmallVector<mlir::Value> &bounds) {
+ llvm::SmallVector<mlir::Value> &bounds, bool treatIndexAsSection = false) {
mlir::Value baseAddr;
std::visit(
@@ -778,7 +798,7 @@ mlir::Value gatherDataOperandAddrAndBounds(
[&](const Fortran::parser::Designator &designator) {
if (auto expr{Fortran::semantics::AnalyzeExpr(semanticsContext,
designator)}) {
- if ((*expr).Rank() > 0 &&
+ if (((*expr).Rank() > 0 || treatIndexAsSection) &&
Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(
designator)) {
const auto *arrayElement =
@@ -809,7 +829,8 @@ mlir::Value gatherDataOperandAddrAndBounds(
asFortran << '(';
bounds = genBoundsOps<BoundsType, BoundsOp>(
builder, operandLocation, converter, stmtCtx,
- arrayElement->subscripts, asFortran, dataExv, baseAddr);
+ arrayElement->subscripts, asFortran, dataExv, baseAddr,
+ treatIndexAsSection);
}
asFortran << ')';
} else if (Fortran::parser::Unwrap<
@@ -845,6 +866,10 @@ mlir::Value gatherDataOperandAddrAndBounds(
if (Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(
designator)) {
// Single array element.
+ const auto *arrayElement =
+ Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(
+ designator);
+ (void)arrayElement;
fir::ExtendedValue compExv =
converter.genExprAddr(operandLocation, *expr, stmtCtx);
baseAddr = fir::getBase(compExv);
diff --git a/flang/lib/Lower/OpenACC.cpp b/flang/lib/Lower/OpenACC.cpp
index 809fd3b3be7cfdf..616580b3d4be2d0 100644
--- a/flang/lib/Lower/OpenACC.cpp
+++ b/flang/lib/Lower/OpenACC.cpp
@@ -264,7 +264,8 @@ genDataOperandOperations(const Fortran::parser::AccObjectList &objectList,
mlir::Value baseAddr = Fortran::lower::gatherDataOperandAddrAndBounds<
Fortran::parser::AccObject, mlir::acc::DataBoundsType,
mlir::acc::DataBoundsOp>(converter, builder, semanticsContext, stmtCtx,
- accObject, operandLocation, asFortran, bounds);
+ accObject, operandLocation, asFortran, bounds,
+ /*treatIndexAsSection=*/true);
Op op = createDataEntryOp<Op>(builder, operandLocation, baseAddr, asFortran,
bounds, structured, implicit, dataClause,
baseAddr.getType());
diff --git a/flang/test/Lower/OpenACC/acc-enter-data.f90 b/flang/test/Lower/OpenACC/acc-enter-data.f90
index 59de2071939eabe..a1f568f38af731c 100644
--- a/flang/test/Lower/OpenACC/acc-enter-data.f90
+++ b/flang/test/Lower/OpenACC/acc-enter-data.f90
@@ -813,7 +813,20 @@ subroutine acc_enter_data_single_array_element()
!$acc enter data create(e(2)%a(1,2))
-!CHECK: %[[CREATE:.*]] = acc.create varPtr(%{{.*}} : !fir.ref<f32>) -> !fir.ref<f32> {name = "e(2_8)%a(1_8,2_8)", structured = false}
-!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.ref<f32>)
+!CHECK-LABEL: func.func @_QPacc_enter_data_single_array_element() {
+!CHECK-DAG: %[[VAL_38:.*]]:3 = fir.box_dims %[[BOX:.*]], %[[VAL_37:.*]] : (!fir.box<!fir.heap<!fir.array<?x?xf32>>>, index) -> (index, index, index)
+!CHECK-DAG: %[[VAL_37]] = arith.constant 0 : index
+!CHECK-DAG: %[[VAL_40:.*]]:3 = fir.box_dims %[[BOX]], %[[VAL_39:.*]] : (!fir.box<!fir.heap<!fir.array<?x?xf32>>>, index) -> (index, index, index)
+!CHECK-DAG: %[[VAL_39]] = arith.constant 1 : index
+!CHECK-DAG: %[[VAL_41:.*]] = fir.box_addr %[[BOX]] : (!fir.box<!fir.heap<!fir.array<?x?xf32>>>) -> !fir.heap<!fir.array<?x?xf32>>
+!CHECK: %[[VAL_42:.*]] = arith.constant 1 : index
+!CHECK: %[[VAL_43:.*]] = arith.constant 1 : index
+!CHECK: %[[VAL_44:.*]] = arith.subi %[[VAL_43]], %[[VAL_38]]#0 : index
+!CHECK: %[[VAL_45:.*]] = acc.bounds lowerbound(%[[VAL_44]] : index) upperbound(%[[VAL_44]] : index) extent(%[[VAL_42]] : index) stride(%[[VAL_42]] : index) startIdx(%[[VAL_38]]#0 : index)
+!CHECK: %[[VAL_46:.*]] = arith.constant 2 : index
+!CHECK: %[[VAL_47:.*]] = arith.subi %[[VAL_46]], %[[VAL_40]]#0 : index
+!CHECK: %[[VAL_48:.*]] = acc.bounds lowerbound(%[[VAL_47]] : index) upperbound(%[[VAL_47]] : index) extent(%[[VAL_42]] : index) stride(%[[VAL_42]] : index) startIdx(%[[VAL_40]]#0 : index)
+!CHECK: %[[CREATE:.*]] = acc.create varPtr(%[[VAL_41]] : !fir.heap<!fir.array<?x?xf32>>) bounds(%[[VAL_45]], %[[VAL_48]]) -> !fir.heap<!fir.array<?x?xf32>> {name = "e(2_8)%a(1,2)", structured = false}
+!CHECK: acc.enter_data dataOperands(%[[CREATE]] : !fir.heap<!fir.array<?x?xf32>>)
end subroutine
|
|
I think it should be fine to toggle this on for OpenMP, I agree using the same method for both dialects would be ideal. Although I have not tested this particular case, I imagine we'd still want the bounds as well. However, could we maintain the on/off toggle (switched on for OpenMP/OpenACC, so perhaps flip the default to true) until this particular use-case has been tested in OpenMP a little down the line from now? If that's unreasonable that's fine, we can likely re-introduce the toggle on the small chance it's an issue in OpenMP! |
clementval
approved these changes
Nov 6, 2023
Thanks! I added a switch for OpenMP only. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
agozillon
approved these changes
Nov 6, 2023
There was a problem hiding this comment.
I was more meaning I think we could just pass true for now in OpenMP similarly to the way it's done in OpenACC and we could just pass false in OpenMP if we ever run into an issue at some point in the future (sorry for not being clear)!
However, this works too if you're happy with it, thank you very much for your work.
This was referenced Nov 8, 2023
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.
In cases like
copy(array(N))it is still useful to representthe data operand uniformly with
copy(array(N:N)).This change generates data bounds even if it is not an array
section with the triplets. The lower and the upper bounds
are the same and the extent is one in this case.
I did not enable this for OpenMP by default, because I am not sure
if this is what the OpenMP devs want (though, I hope we can use the same).