[flang] Inline hlfir.eoshift during HLFIR intrinsics simplication. #153108
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
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Slava Zakharin (vzakhari) ChangesThis patch generalizes the code for hlfir.cshift to be applicable Patch is 237.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/153108.diff 8 Files Affected:
diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td b/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td
index ee0b5aa9760b1..0bddfd85d436b 100644
--- a/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td
+++ b/flang/include/flang/Optimizer/HLFIR/HLFIROpBase.td
@@ -95,9 +95,9 @@ def IsFortranValuePred : CPred<"::hlfir::isFortranValueType($_self)">;
def AnyFortranValue
: TypeConstraint<IsFortranValuePred, "any Fortran value type">;
-
-def AnyFortranEntity : TypeConstraint<Or<[AnyFortranVariable.predicate,
- AnyFortranValue.predicate]>, "any Fortran value or variable type">;
+def AnyFortranEntity
+ : Type<Or<[AnyFortranVariable.predicate, AnyFortranValue.predicate]>,
+ "any Fortran value or variable type">;
def IsFortranScalarCharacterPred
: CPred<"::hlfir::isFortranScalarCharacterType($_self)">;
diff --git a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
index 2f5da720fbe1d..db3fb0b90464d 100644
--- a/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
+++ b/flang/include/flang/Optimizer/HLFIR/HLFIROps.td
@@ -721,6 +721,28 @@ def hlfir_CShiftOp
let hasVerifier = 1;
}
+def hlfir_EOShiftOp
+ : hlfir_Op<
+ "eoshift", [AttrSizedOperandSegments,
+ DeclareOpInterfaceMethods<MemoryEffectsOpInterface>]> {
+ let summary = "EOSHIFT transformational intrinsic";
+ let description = [{
+ End-off shift of an array
+ }];
+
+ let arguments = (ins AnyFortranArrayObject:$array,
+ AnyFortranIntegerScalarOrArrayObject:$shift,
+ Optional<AnyFortranEntity>:$boundary, Optional<AnyIntegerType>:$dim);
+
+ let results = (outs hlfir_ExprType);
+
+ let assemblyFormat = [{
+ $array $shift (`boundary` $boundary^)? (`dim` $dim^)? attr-dict `:` functional-type(operands, results)
+ }];
+
+ let hasVerifier = 1;
+}
+
def hlfir_ReshapeOp
: hlfir_Op<
"reshape", [AttrSizedOperandSegments,
diff --git a/flang/lib/Optimizer/Builder/HLFIRTools.cpp b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
index b6d692a0226cd..086dd66711602 100644
--- a/flang/lib/Optimizer/Builder/HLFIRTools.cpp
+++ b/flang/lib/Optimizer/Builder/HLFIRTools.cpp
@@ -416,7 +416,10 @@ hlfir::Entity hlfir::loadTrivialScalar(mlir::Location loc,
entity = derefPointersAndAllocatables(loc, builder, entity);
if (entity.isVariable() && entity.isScalar() &&
fir::isa_trivial(entity.getFortranElementType())) {
- return Entity{fir::LoadOp::create(builder, loc, entity)};
+ // Optional entities may be represented with !fir.box<i32/f32/...>.
+ // We need to take the data pointer before loading the scalar.
+ mlir::Value base = genVariableRawAddress(loc, builder, entity);
+ return Entity{fir::LoadOp::create(builder, loc, base)};
}
return entity;
}
diff --git a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
index ed102db69dae3..93ee94a120aa1 100644
--- a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
+++ b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
@@ -1440,44 +1440,46 @@ void hlfir::MatmulTransposeOp::getEffects(
}
//===----------------------------------------------------------------------===//
-// CShiftOp
+// Array shifts: CShiftOp/EOShiftOp
//===----------------------------------------------------------------------===//
-llvm::LogicalResult hlfir::CShiftOp::verify() {
- mlir::Value array = getArray();
+template <typename Op>
+static llvm::LogicalResult verifyArrayShift(Op op) {
+ mlir::Value array = op.getArray();
fir::SequenceType arrayTy = mlir::cast<fir::SequenceType>(
hlfir::getFortranElementOrSequenceType(array.getType()));
llvm::ArrayRef<int64_t> inShape = arrayTy.getShape();
std::size_t arrayRank = inShape.size();
mlir::Type eleTy = arrayTy.getEleTy();
- hlfir::ExprType resultTy = mlir::cast<hlfir::ExprType>(getResult().getType());
+ hlfir::ExprType resultTy =
+ mlir::cast<hlfir::ExprType>(op.getResult().getType());
llvm::ArrayRef<int64_t> resultShape = resultTy.getShape();
std::size_t resultRank = resultShape.size();
mlir::Type resultEleTy = resultTy.getEleTy();
- mlir::Value shift = getShift();
+ mlir::Value shift = op.getShift();
mlir::Type shiftTy = hlfir::getFortranElementOrSequenceType(shift.getType());
- // TODO: turn allowCharacterLenMismatch into true.
- if (auto match = areMatchingTypes(*this, eleTy, resultEleTy,
- /*allowCharacterLenMismatch=*/false);
+ if (auto match = areMatchingTypes(
+ op, eleTy, resultEleTy,
+ /*allowCharacterLenMismatch=*/!useStrictIntrinsicVerifier);
match.failed())
- return emitOpError(
+ return op.emitOpError(
"input and output arrays should have the same element type");
if (arrayRank != resultRank)
- return emitOpError("input and output arrays should have the same rank");
+ return op.emitOpError("input and output arrays should have the same rank");
constexpr int64_t unknownExtent = fir::SequenceType::getUnknownExtent();
for (auto [inDim, resultDim] : llvm::zip(inShape, resultShape))
if (inDim != unknownExtent && resultDim != unknownExtent &&
inDim != resultDim)
- return emitOpError(
+ return op.emitOpError(
"output array's shape conflicts with the input array's shape");
int64_t dimVal = -1;
- if (!getDim())
+ if (!op.getDim())
dimVal = 1;
- else if (auto dim = fir::getIntIfConstant(getDim()))
+ else if (auto dim = fir::getIntIfConstant(op.getDim()))
dimVal = *dim;
// The DIM argument may be statically invalid (e.g. exceed the
@@ -1485,44 +1487,79 @@ llvm::LogicalResult hlfir::CShiftOp::verify() {
// so avoid some checks unless useStrictIntrinsicVerifier is true.
if (useStrictIntrinsicVerifier && dimVal != -1) {
if (dimVal < 1)
- return emitOpError("DIM must be >= 1");
+ return op.emitOpError("DIM must be >= 1");
if (dimVal > static_cast<int64_t>(arrayRank))
- return emitOpError("DIM must be <= input array's rank");
+ return op.emitOpError("DIM must be <= input array's rank");
}
- if (auto shiftSeqTy = mlir::dyn_cast<fir::SequenceType>(shiftTy)) {
- // SHIFT is an array. Verify the rank and the shape (if DIM is constant).
- llvm::ArrayRef<int64_t> shiftShape = shiftSeqTy.getShape();
- std::size_t shiftRank = shiftShape.size();
- if (shiftRank != arrayRank - 1)
- return emitOpError(
- "SHIFT's rank must be 1 less than the input array's rank");
-
- if (useStrictIntrinsicVerifier && dimVal != -1) {
- // SHIFT's shape must be [d(1), d(2), ..., d(DIM-1), d(DIM+1), ..., d(n)],
- // where [d(1), d(2), ..., d(n)] is the shape of the ARRAY.
- int64_t arrayDimIdx = 0;
- int64_t shiftDimIdx = 0;
- for (auto shiftDim : shiftShape) {
- if (arrayDimIdx == dimVal - 1)
+ // A helper lambda to verify the shape of the array types of
+ // certain operands of the array shift (e.g. the SHIFT and BOUNDARY operands).
+ auto verifyOperandTypeShape = [&](mlir::Type type,
+ llvm::Twine name) -> llvm::LogicalResult {
+ if (auto opndSeqTy = mlir::dyn_cast<fir::SequenceType>(type)) {
+ // The operand is an array. Verify the rank and the shape (if DIM is
+ // constant).
+ llvm::ArrayRef<int64_t> opndShape = opndSeqTy.getShape();
+ std::size_t opndRank = opndShape.size();
+ if (opndRank != arrayRank - 1)
+ return op.emitOpError(
+ name + "'s rank must be 1 less than the input array's rank");
+
+ if (useStrictIntrinsicVerifier && dimVal != -1) {
+ // The operand's shape must be
+ // [d(1), d(2), ..., d(DIM-1), d(DIM+1), ..., d(n)],
+ // where [d(1), d(2), ..., d(n)] is the shape of the ARRAY.
+ int64_t arrayDimIdx = 0;
+ int64_t opndDimIdx = 0;
+ for (auto opndDim : opndShape) {
+ if (arrayDimIdx == dimVal - 1)
+ ++arrayDimIdx;
+
+ if (inShape[arrayDimIdx] != unknownExtent &&
+ opndDim != unknownExtent && inShape[arrayDimIdx] != opndDim)
+ return op.emitOpError("SHAPE(ARRAY)(" +
+ llvm::Twine(arrayDimIdx + 1) +
+ ") must be equal to SHAPE(" + name + ")(" +
+ llvm::Twine(opndDimIdx + 1) +
+ "): " + llvm::Twine(inShape[arrayDimIdx]) +
+ " != " + llvm::Twine(opndDim));
++arrayDimIdx;
-
- if (inShape[arrayDimIdx] != unknownExtent &&
- shiftDim != unknownExtent && inShape[arrayDimIdx] != shiftDim)
- return emitOpError("SHAPE(ARRAY)(" + llvm::Twine(arrayDimIdx + 1) +
- ") must be equal to SHAPE(SHIFT)(" +
- llvm::Twine(shiftDimIdx + 1) +
- "): " + llvm::Twine(inShape[arrayDimIdx]) +
- " != " + llvm::Twine(shiftDim));
- ++arrayDimIdx;
- ++shiftDimIdx;
+ ++opndDimIdx;
+ }
}
}
+ return mlir::success();
+ };
+
+ if (failed(verifyOperandTypeShape(shiftTy, "SHIFT")))
+ return mlir::failure();
+
+ if constexpr (std::is_same_v<Op, hlfir::EOShiftOp>) {
+ if (mlir::Value boundary = op.getBoundary()) {
+ mlir::Type boundaryTy =
+ hlfir::getFortranElementOrSequenceType(boundary.getType());
+ if (auto match = areMatchingTypes(
+ op, eleTy, hlfir::getFortranElementType(boundaryTy),
+ /*allowCharacterLenMismatch=*/!useStrictIntrinsicVerifier);
+ match.failed())
+ return op.emitOpError(
+ "ARRAY and BOUNDARY operands must have the same element type");
+ if (failed(verifyOperandTypeShape(boundaryTy, "BOUNDARY")))
+ return mlir::failure();
+ }
}
return mlir::success();
}
+//===----------------------------------------------------------------------===//
+// CShiftOp
+//===----------------------------------------------------------------------===//
+
+llvm::LogicalResult hlfir::CShiftOp::verify() {
+ return verifyArrayShift(*this);
+}
+
void hlfir::CShiftOp::getEffects(
llvm::SmallVectorImpl<
mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
@@ -1530,6 +1567,21 @@ void hlfir::CShiftOp::getEffects(
getIntrinsicEffects(getOperation(), effects);
}
+//===----------------------------------------------------------------------===//
+// EOShiftOp
+//===----------------------------------------------------------------------===//
+
+llvm::LogicalResult hlfir::EOShiftOp::verify() {
+ return verifyArrayShift(*this);
+}
+
+void hlfir::EOShiftOp::getEffects(
+ llvm::SmallVectorImpl<
+ mlir::SideEffects::EffectInstance<mlir::MemoryEffects::Effect>>
+ &effects) {
+ getIntrinsicEffects(getOperation(), effects);
+}
+
//===----------------------------------------------------------------------===//
// ReshapeOp
//===----------------------------------------------------------------------===//
@@ -1543,7 +1595,8 @@ llvm::LogicalResult hlfir::ReshapeOp::verify() {
hlfir::getFortranElementOrSequenceType(array.getType()));
if (auto match = areMatchingTypes(
*this, hlfir::getFortranElementType(resultType),
- arrayType.getElementType(), /*allowCharacterLenMismatch=*/true);
+ arrayType.getElementType(),
+ /*allowCharacterLenMismatch=*/!useStrictIntrinsicVerifier);
match.failed())
return emitOpError("ARRAY and the result must have the same element type");
if (hlfir::isPolymorphicType(resultType) !=
@@ -1565,9 +1618,9 @@ llvm::LogicalResult hlfir::ReshapeOp::verify() {
if (mlir::Value pad = getPad()) {
auto padArrayType = mlir::cast<fir::SequenceType>(
hlfir::getFortranElementOrSequenceType(pad.getType()));
- if (auto match = areMatchingTypes(*this, arrayType.getElementType(),
- padArrayType.getElementType(),
- /*allowCharacterLenMismatch=*/true);
+ if (auto match = areMatchingTypes(
+ *this, arrayType.getElementType(), padArrayType.getElementType(),
+ /*allowCharacterLenMismatch=*/!useStrictIntrinsicVerifier);
match.failed())
return emitOpError("ARRAY and PAD must be of the same type");
}
diff --git a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
index b27c3a8526945..ac3b62055d37a 100644
--- a/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
+++ b/flang/lib/Optimizer/HLFIR/Transforms/SimplifyHLFIRIntrinsics.cpp
@@ -10,6 +10,7 @@
// into the calling function.
//===----------------------------------------------------------------------===//
+#include "flang/Optimizer/Builder/Character.h"
#include "flang/Optimizer/Builder/Complex.h"
#include "flang/Optimizer/Builder/FIRBuilder.h"
#include "flang/Optimizer/Builder/HLFIRTools.h"
@@ -1269,64 +1270,91 @@ class ReductionConversion : public mlir::OpRewritePattern<Op> {
}
};
-class CShiftConversion : public mlir::OpRewritePattern<hlfir::CShiftOp> {
+template <typename Op>
+class ArrayShiftConversion : public mlir::OpRewritePattern<Op> {
public:
- using mlir::OpRewritePattern<hlfir::CShiftOp>::OpRewritePattern;
+ // The implementation below only support CShiftOp and EOShiftOp.
+ static_assert(std::is_same_v<Op, hlfir::CShiftOp> ||
+ std::is_same_v<Op, hlfir::EOShiftOp>);
+
+ using mlir::OpRewritePattern<Op>::OpRewritePattern;
llvm::LogicalResult
- matchAndRewrite(hlfir::CShiftOp cshift,
- mlir::PatternRewriter &rewriter) const override {
+ matchAndRewrite(Op op, mlir::PatternRewriter &rewriter) const override {
- hlfir::ExprType expr = mlir::dyn_cast<hlfir::ExprType>(cshift.getType());
+ hlfir::ExprType expr = mlir::dyn_cast<hlfir::ExprType>(op.getType());
assert(expr &&
- "expected an expression type for the result of hlfir.cshift");
+ "expected an expression type for the result of the array shift");
unsigned arrayRank = expr.getRank();
- // When it is a 1D CSHIFT, we may assume that the DIM argument
+ // When it is a 1D CSHIFT/EOSHIFT, we may assume that the DIM argument
// (whether it is present or absent) is equal to 1, otherwise,
// the program is illegal.
int64_t dimVal = 1;
if (arrayRank != 1)
- if (mlir::Value dim = cshift.getDim()) {
+ if (mlir::Value dim = op.getDim()) {
auto constDim = fir::getIntIfConstant(dim);
if (!constDim)
- return rewriter.notifyMatchFailure(cshift,
- "Nonconstant DIM for CSHIFT");
+ return rewriter.notifyMatchFailure(
+ op, "Nonconstant DIM for CSHIFT/EOSHIFT");
dimVal = *constDim;
}
if (dimVal <= 0 || dimVal > arrayRank)
- return rewriter.notifyMatchFailure(cshift, "Invalid DIM for CSHIFT");
+ return rewriter.notifyMatchFailure(op, "Invalid DIM for CSHIFT/EOSHIFT");
+
+ if constexpr (std::is_same_v<Op, hlfir::EOShiftOp>) {
+ // TODO: the EOSHIFT inlining code is not ready to produce
+ // fir.if selecting between ARRAY and BOUNDARY (or the default
+ // boundary value), when they are expressions of type CHARACTER.
+ // This needs more work.
+ if (mlir::isa<fir::CharacterType>(expr.getEleTy())) {
+ if (!hlfir::Entity{op.getArray()}.isVariable())
+ return rewriter.notifyMatchFailure(
+ op, "EOSHIFT with ARRAY being CHARACTER expression");
+ if (op.getBoundary() && !hlfir::Entity{op.getBoundary()}.isVariable())
+ return rewriter.notifyMatchFailure(
+ op, "EOSHIFT with BOUNDARY being CHARACTER expression");
+ }
+ // TODO: selecting between ARRAY and BOUNDARY values with derived types
+ // need more work.
+ if (fir::isa_derived(expr.getEleTy()))
+ return rewriter.notifyMatchFailure(op, "EOSHIFT of derived type");
+ }
// When DIM==1 and the contiguity of the input array is not statically
// known, try to exploit the fact that the leading dimension might be
// contiguous. We can do this now using hlfir.eval_in_mem with
// a dynamic check for the leading dimension contiguity.
- // Otherwise, convert hlfir.cshift to hlfir.elemental.
+ // Otherwise, convert hlfir.cshift/eoshift to hlfir.elemental.
//
// Note that the hlfir.elemental can be inlined into other hlfir.elemental,
// while hlfir.eval_in_mem prevents this, and we will end up creating
// a temporary array for the result. We may need to come up with
// a more sophisticated logic for picking the most efficient
// representation.
- hlfir::Entity array = hlfir::Entity{cshift.getArray()};
+ hlfir::Entity array = hlfir::Entity{op.getArray()};
mlir::Type elementType = array.getFortranElementType();
if (dimVal == 1 && fir::isa_trivial(elementType) &&
- // genInMemCShift() only works for variables currently.
+ // genInMemArrayShift() only works for variables currently.
array.isVariable())
- rewriter.replaceOp(cshift, genInMemCShift(rewriter, cshift, dimVal));
+ rewriter.replaceOp(op, genInMemArrayShift(rewriter, op, dimVal));
else
- rewriter.replaceOp(cshift, genElementalCShift(rewriter, cshift, dimVal));
+ rewriter.replaceOp(op, genElementalArrayShift(rewriter, op, dimVal));
return mlir::success();
}
private:
- /// Generate MODULO(\p shiftVal, \p extent).
+ /// For CSHIFT, generate MODULO(\p shiftVal, \p extent).
+ /// For EOSHIFT, return \p shiftVal casted to \p calcType.
static mlir::Value normalizeShiftValue(mlir::Location loc,
fir::FirOpBuilder &builder,
mlir::Value shiftVal,
mlir::Value extent,
mlir::Type calcType) {
shiftVal = builder.createConvert(loc, calcType, shiftVal);
+ if constexpr (std::is_same_v<Op, hlfir::EOShiftOp>)
+ return shiftVal;
+
extent = builder.createConvert(loc, calcType, extent);
// Make sure that we do not divide by zero. When the dimension
// has zero size, turn the extent into 1. Note that the computed
@@ -1342,24 +1370,221 @@ class CShiftConversion : public mlir::OpRewritePattern<hlfir::CShiftOp> {
return builder.createConvert(loc, calcType, shiftVal);
}
- /// Convert \p cshift into an hlfir.elemental using
+ /// The indices computations for the array shifts are done using I64 type.
+ /// For CSHIFT, all computations do not overflow signed and unsigned I64.
+ /// For EOSHIFT, some computations may involve negative shift values,
+ /// so using no-unsigned wrap flag would be incorrect.
+ static void setArithOverflowFlags(Op op, fir::FirOpBuilder &builder) {
+ if constexpr (std::is_same_v<Op, hlfir::EOShiftOp>)
+ builder.setIntegerOverflowFlags(mlir::arith::IntegerOverflowFlags::nsw);
+ else
+ builder.setIntegerOverflowFlags(mlir::arith::IntegerOverflowFlags::nsw |
+ mlir::arith::IntegerOverflowFlags::nuw);
+ }
+
+ /// Return the element type of the EOSHIFT boundary that may be omitted
+ /// statically or dynamically. This element type might be used
+ /// to generate MLIR where we have to select between the default
+ /// boundary value and the dynamically absent/present boundary value.
+ /// If the boundary has a type not defined in Table 16.4 in 16.9.77
+ /// of F2023, then the return value is nullptr.
+ static mlir::Type getDefaultBoundaryValueType(mlir::Type elementType) {
+ // To be able to generate a "select" between the default boundary value
+ // and the dynamic boundary value, use BoxCharType for the CHARACTER
+ // cases. This might be a little bit inefficient, because we may
+ // create unnecessary tuples, but it simplifies the inlining code.
+ if (auto charTy = mlir::dyn_cast<fir::CharacterType>(elementType))
+ return fir::BoxCharType::get(charTy.getContext(), charTy.getFKind());
+
+ if (mlir::isa<fir::LogicalType>(elementType) ||
+ fir::isa_integer(elementType) || fir::isa_real(elementType) ||
+ fir::isa_complex(elementType)...
[truncated]
|
|
This change improves |
|
This PR includes the basic op definition from #153105. |
tblah
reviewed
Aug 13, 2025
Comment on lines
1544
to
1560
There was a problem hiding this comment.
It feels like precomputedScalarBoundary is encoding too many things at once, making it confusing. Could we split it maybe into a value that only stores scalars and a boolean that says whether the boundary value might be dynamically absent? (Apologies if I have misunderstood this)
There was a problem hiding this comment.
Sorry if the comments are confusing.
precomputedScalarBoundary actually encodes just one thing that is the pre-computed scalar boundary, if it is ever going to be needed later in the "kernel" code.
It is not null, when:
- The op's boundary is absent (implying that the data type allows absent boundary).
- The op's boundary is scalar (regardless of its dynamic presence).
- The op's boundary is an array that may be dynamically absent and the data type allows absent boundary.
Do you suggest adding a boolean that will be set to true only in the last case?
There was a problem hiding this comment.
Ahh thank you for explaining. What I find confusing here is that a null boundary value can mean different things depending on the data type.
If I understand correctly, one way of expressing how it is used is to say that it is the default value to use when the boundary is absent and the scalar value to use regardless. So I guess precomputedScalarBoundary is a good name.
I found the logic of this function a bit confusing to read in isolation but yes when referencing how the value is generated it makes perfect sense. Perhaps just expanding the logic out could be clearer? Something like
// No boundary: a default value has been precomputed.
if (!op.getBoundary())
return precomputedScalarBoundary;
// Scalar case: boundary value does not depend upon the indices
// and so it can be precomputed.
hlfir::Entity boundary{op.getBoundary()};
if (boundary.isScalar())
return precomputedScalarBoundary;
// If the boundary is statically present and not scalar then we don't
// need to precompute any boundary value and so it will be null.
bool mustBePresent = !precomputedScalarBoundary;
if (mustBePresent) {
// The array boundary must be present, so we just need to load
// the scalar boundary value.
return loadEoshiftVal(loc, builder, boundary, oneBasedIndices);
}
// The array boundary may be dynamically absent.
// In this case, precomputedScalarBoundary is a pre-computed scalar
// boundary value that has to be used if boundaryIsScalarPred
// is true, otherwise, the boundary value has to be loaded
// from the boundary array.
I hope this doesn't feel too pedantic. It is possible I was just tired reviewing this and so it seemed harder to follow than it really is.
tblah
reviewed
Aug 14, 2025
Comment on lines
1544
to
1560
There was a problem hiding this comment.
Ahh thank you for explaining. What I find confusing here is that a null boundary value can mean different things depending on the data type.
If I understand correctly, one way of expressing how it is used is to say that it is the default value to use when the boundary is absent and the scalar value to use regardless. So I guess precomputedScalarBoundary is a good name.
I found the logic of this function a bit confusing to read in isolation but yes when referencing how the value is generated it makes perfect sense. Perhaps just expanding the logic out could be clearer? Something like
// No boundary: a default value has been precomputed.
if (!op.getBoundary())
return precomputedScalarBoundary;
// Scalar case: boundary value does not depend upon the indices
// and so it can be precomputed.
hlfir::Entity boundary{op.getBoundary()};
if (boundary.isScalar())
return precomputedScalarBoundary;
// If the boundary is statically present and not scalar then we don't
// need to precompute any boundary value and so it will be null.
bool mustBePresent = !precomputedScalarBoundary;
if (mustBePresent) {
// The array boundary must be present, so we just need to load
// the scalar boundary value.
return loadEoshiftVal(loc, builder, boundary, oneBasedIndices);
}
// The array boundary may be dynamically absent.
// In this case, precomputedScalarBoundary is a pre-computed scalar
// boundary value that has to be used if boundaryIsScalarPred
// is true, otherwise, the boundary value has to be loaded
// from the boundary array.
I hope this doesn't feel too pedantic. It is possible I was just tired reviewing this and so it seemed harder to follow than it really is.
There was a problem hiding this comment.
Suggested change
| // they are properly updated between the labda calls. | |
| // they are properly updated between the lambda calls. |
tblah
approved these changes
Aug 15, 2025
This patch generalizes the code for hlfir.cshift to be applicable for hlfir.eoshift. The major difference is the selection of the boundary value that might be statically/dynamically absent, in which case the default scalar value has to be used. The scalar value of the boundary is always computed before the hlfir.elemental or the assignment loop. Contrary to hlfir.cshift simplication, the SHIFT value is not normalized, because the original value (and its sign) participate in the EOSHIFT index computation for addressing the input array and selecting which elements of the results are assigned from the boundary operand.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This patch generalizes the code for hlfir.cshift to be applicable
for hlfir.eoshift. The major difference is the selection
of the boundary value that might be statically/dynamically absent,
in which case the default scalar value has to be used.
The scalar value of the boundary is always computed before
the hlfir.elemental or the assignment loop.
Contrary to hlfir.cshift simplication, the SHIFT value is not normalized,
because the original value (and its sign) participate in the EOSHIFT
index computation for addressing the input array and selecting
which elements of the results are assigned from the boundary operand.