[flang][HLFIR] Relax verifiers of intrinsic operations #80132
Merged
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
The verifiers are currently very strict: requiring intrinsic operations to be used only in cases where the Fortran standard permits the intrinsic to be used. There have now been a lot of cases where these verifiers have caused bugs in corner cases. In a recent ticket, @jeanPerier pointed out that it could be useful for future optimizations if somewhat invalid uses of these operations could be allowed in dead code. See this comment: llvm#79995 (comment) In response to all of this, I have decided to relax the intrinsic operation verifiers. The intention is now to only disallow operation uses that are likely to crash the compiler. It isn't obvious which checks are more useful than others, and it would not be a good use of time to try lots of illegal (according to Fortran) intrinsic uses to figure out exactly which can crash Flang (as none of these should pass semantics anyway). I have taken an educated guess. The disadvantage of this approach is that IR can now represent intrinsic invocations which are incorrect. The lowering and implementation of these intrinsic functions is unlikely to do the right thing in all of these cases, and as they should mostly be impossible to generate using normal Fortran code, these edge cases will see very little testing, before some new optimization causes them to become more common. Fixes llvm#79995
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Tom Eccles (tblah) ChangesThe verifiers are currently very strict: requiring intrinsic operations to be used only in cases where the Fortran standard permits the intrinsic to be used. There have now been a lot of cases where these verifiers have caused bugs in corner cases. In a recent ticket, @jeanPerier pointed out that it could be useful for future optimizations if somewhat invalid uses of these operations could be allowed in dead code. See this comment: #79995 (comment) In response to all of this, I have decided to relax the intrinsic operation verifiers. The intention is now to only disallow operation uses that are likely to crash the compiler. It isn't obvious which checks are more useful than others, and it would not be a good use of time to try lots of illegal (according to Fortran) intrinsic uses to figure out exactly which can crash Flang (as none of these should pass semantics anyway). I have taken an educated guess. The disadvantage of this approach is that IR can now represent intrinsic invocations which are incorrect. The lowering and implementation of these intrinsic functions is unlikely to do the right thing in all of these cases, and as they should mostly be impossible to generate using normal Fortran code, these edge cases will see very little testing, before some new optimization causes them to become more common. Fixes #79995 Patch is 47.63 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/80132.diff 3 Files Affected:
diff --git a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
index 439d106d0bfed..c247b7d486c6c 100644
--- a/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
+++ b/flang/lib/Optimizer/HLFIR/IR/HLFIROps.cpp
@@ -475,65 +475,11 @@ mlir::LogicalResult hlfir::ParentComponentOp::verify() {
return mlir::success();
}
-//===----------------------------------------------------------------------===//
-// LogicalReductionOp
-//===----------------------------------------------------------------------===//
-template <typename LogicalReductionOp>
-static mlir::LogicalResult
-verifyLogicalReductionOp(LogicalReductionOp reductionOp) {
- mlir::Operation *op = reductionOp->getOperation();
-
- auto results = op->getResultTypes();
- assert(results.size() == 1);
-
- mlir::Value mask = reductionOp->getMask();
- mlir::Value dim = reductionOp->getDim();
-
- fir::SequenceType maskTy =
- hlfir::getFortranElementOrSequenceType(mask.getType())
- .cast<fir::SequenceType>();
- mlir::Type logicalTy = maskTy.getEleTy();
- llvm::ArrayRef<int64_t> maskShape = maskTy.getShape();
-
- mlir::Type resultType = results[0];
- if (mlir::isa<fir::LogicalType>(resultType)) {
- // Result is of the same type as MASK
- if (resultType != logicalTy)
- return reductionOp->emitOpError(
- "result must have the same element type as MASK argument");
-
- } else if (auto resultExpr =
- mlir::dyn_cast_or_null<hlfir::ExprType>(resultType)) {
- // Result should only be in hlfir.expr form if it is an array
- if (maskShape.size() > 1 && dim != nullptr) {
- if (!resultExpr.isArray())
- return reductionOp->emitOpError("result must be an array");
-
- if (resultExpr.getEleTy() != logicalTy)
- return reductionOp->emitOpError(
- "result must have the same element type as MASK argument");
-
- llvm::ArrayRef<int64_t> resultShape = resultExpr.getShape();
- // Result has rank n-1
- if (resultShape.size() != (maskShape.size() - 1))
- return reductionOp->emitOpError(
- "result rank must be one less than MASK");
- } else {
- return reductionOp->emitOpError("result must be of logical type");
- }
- } else {
- return reductionOp->emitOpError("result must be of logical type");
- }
- return mlir::success();
-}
-
//===----------------------------------------------------------------------===//
// AllOp
//===----------------------------------------------------------------------===//
-mlir::LogicalResult hlfir::AllOp::verify() {
- return verifyLogicalReductionOp<hlfir::AllOp *>(this);
-}
+mlir::LogicalResult hlfir::AllOp::verify() { return mlir::success(); }
void hlfir::AllOp::getEffects(
llvm::SmallVectorImpl<
@@ -546,9 +492,7 @@ void hlfir::AllOp::getEffects(
// AnyOp
//===----------------------------------------------------------------------===//
-mlir::LogicalResult hlfir::AnyOp::verify() {
- return verifyLogicalReductionOp<hlfir::AnyOp *>(this);
-}
+mlir::LogicalResult hlfir::AnyOp::verify() { return mlir::success(); }
void hlfir::AnyOp::getEffects(
llvm::SmallVectorImpl<
@@ -561,38 +505,7 @@ void hlfir::AnyOp::getEffects(
// CountOp
//===----------------------------------------------------------------------===//
-mlir::LogicalResult hlfir::CountOp::verify() {
- mlir::Operation *op = getOperation();
-
- auto results = op->getResultTypes();
- assert(results.size() == 1);
- mlir::Value mask = getMask();
- mlir::Value dim = getDim();
-
- fir::SequenceType maskTy =
- hlfir::getFortranElementOrSequenceType(mask.getType())
- .cast<fir::SequenceType>();
- llvm::ArrayRef<int64_t> maskShape = maskTy.getShape();
-
- mlir::Type resultType = results[0];
- if (auto resultExpr = mlir::dyn_cast_or_null<hlfir::ExprType>(resultType)) {
- if (maskShape.size() > 1 && dim != nullptr) {
- if (!resultExpr.isArray())
- return emitOpError("result must be an array");
-
- llvm::ArrayRef<int64_t> resultShape = resultExpr.getShape();
- // Result has rank n-1
- if (resultShape.size() != (maskShape.size() - 1))
- return emitOpError("result rank must be one less than MASK");
- } else {
- return emitOpError("result must be of numerical scalar type");
- }
- } else if (!hlfir::isFortranScalarNumericalType(resultType)) {
- return emitOpError("result must be of numerical scalar type");
- }
-
- return mlir::success();
-}
+mlir::LogicalResult hlfir::CountOp::verify() { return mlir::success(); }
void hlfir::CountOp::getEffects(
llvm::SmallVectorImpl<
@@ -682,16 +595,6 @@ verifyArrayAndMaskForReductionOp(NumericalReductionOp reductionOp) {
if (!maskShape.empty()) {
if (maskShape.size() != arrayShape.size())
return reductionOp->emitWarning("MASK must be conformable to ARRAY");
- static_assert(fir::SequenceType::getUnknownExtent() ==
- hlfir::ExprType::getUnknownExtent());
- constexpr int64_t unknownExtent = fir::SequenceType::getUnknownExtent();
- for (std::size_t i = 0; i < arrayShape.size(); ++i) {
- int64_t arrayExtent = arrayShape[i];
- int64_t maskExtent = maskShape[i];
- if ((arrayExtent != maskExtent) && (arrayExtent != unknownExtent) &&
- (maskExtent != unknownExtent))
- return reductionOp->emitWarning("MASK must be conformable to ARRAY");
- }
}
}
return mlir::success();
@@ -710,40 +613,12 @@ verifyNumericalReductionOp(NumericalReductionOp reductionOp) {
mlir::Value array = reductionOp->getArray();
mlir::Value dim = reductionOp->getDim();
- fir::SequenceType arrayTy =
- hlfir::getFortranElementOrSequenceType(array.getType())
- .cast<fir::SequenceType>();
- mlir::Type numTy = arrayTy.getEleTy();
- llvm::ArrayRef<int64_t> arrayShape = arrayTy.getShape();
mlir::Type resultType = results[0];
- if (hlfir::isFortranScalarNumericalType(resultType)) {
- // Result is of the same type as ARRAY
- if (resultType != numTy)
- return reductionOp->emitOpError(
- "result must have the same element type as ARRAY argument");
-
- } else if (auto resultExpr =
- mlir::dyn_cast_or_null<hlfir::ExprType>(resultType)) {
- if (arrayShape.size() > 1 && dim != nullptr) {
- if (!resultExpr.isArray())
- return reductionOp->emitOpError("result must be an array");
-
- if (resultExpr.getEleTy() != numTy)
- return reductionOp->emitOpError(
- "result must have the same element type as ARRAY argument");
-
- llvm::ArrayRef<int64_t> resultShape = resultExpr.getShape();
- // Result has rank n-1
- if (resultShape.size() != (arrayShape.size() - 1))
- return reductionOp->emitOpError(
- "result rank must be one less than ARRAY");
- } else {
- return reductionOp->emitOpError(
- "result must be of numerical scalar type");
- }
- } else {
- return reductionOp->emitOpError("result must be of numerical scalar type");
+ if (!hlfir::isFortranScalarNumericalType(resultType) &&
+ !mlir::isa<hlfir::ExprType>(resultType)) {
+ return reductionOp->emitOpError(
+ "result must be of numerical scalar or array type");
}
return mlir::success();
}
@@ -783,7 +658,6 @@ verifyCharacterReductionOp(CharacterReductionOp reductionOp) {
fir::SequenceType arrayTy =
hlfir::getFortranElementOrSequenceType(array.getType())
.cast<fir::SequenceType>();
- mlir::Type numTy = arrayTy.getEleTy();
llvm::ArrayRef<int64_t> arrayShape = arrayTy.getShape();
auto resultExpr = results[0].cast<hlfir::ExprType>();
@@ -791,19 +665,9 @@ verifyCharacterReductionOp(CharacterReductionOp reductionOp) {
assert(mlir::isa<fir::CharacterType>(resultType) &&
"result must be character");
- // Result is of the same type as ARRAY
- if (resultType != numTy)
- return reductionOp->emitOpError(
- "result must have the same element type as ARRAY argument");
-
if (arrayShape.size() > 1 && dim != nullptr) {
if (!resultExpr.isArray())
return reductionOp->emitOpError("result must be an array");
- llvm::ArrayRef<int64_t> resultShape = resultExpr.getShape();
- // Result has rank n-1
- if (resultShape.size() != (arrayShape.size() - 1))
- return reductionOp->emitOpError(
- "result rank must be one less than ARRAY");
} else if (!resultExpr.isScalar()) {
return reductionOp->emitOpError("result must be scalar character");
}
@@ -823,9 +687,8 @@ mlir::LogicalResult hlfir::MaxvalOp::verify() {
auto resultExpr = mlir::dyn_cast<hlfir::ExprType>(results[0]);
if (resultExpr && mlir::isa<fir::CharacterType>(resultExpr.getEleTy())) {
return verifyCharacterReductionOp<hlfir::MaxvalOp *>(this);
- } else {
- return verifyNumericalReductionOp<hlfir::MaxvalOp *>(this);
}
+ return verifyNumericalReductionOp<hlfir::MaxvalOp *>(this);
}
void hlfir::MaxvalOp::getEffects(
@@ -848,9 +711,8 @@ mlir::LogicalResult hlfir::MinvalOp::verify() {
auto resultExpr = mlir::dyn_cast<hlfir::ExprType>(results[0]);
if (resultExpr && mlir::isa<fir::CharacterType>(resultExpr.getEleTy())) {
return verifyCharacterReductionOp<hlfir::MinvalOp *>(this);
- } else {
- return verifyNumericalReductionOp<hlfir::MinvalOp *>(this);
}
+ return verifyNumericalReductionOp<hlfir::MinvalOp *>(this);
}
void hlfir::MinvalOp::getEffects(
@@ -889,15 +751,6 @@ verifyResultForMinMaxLoc(NumericalReductionOp reductionOp) {
if (!fir::isa_integer(resultExpr.getEleTy()))
return reductionOp->emitOpError("result must have integer elements");
-
- llvm::ArrayRef<int64_t> resultShape = resultExpr.getShape();
- // With dim the result has rank n-1
- if (dim && resultShape.size() != (arrayShape.size() - 1))
- return reductionOp->emitOpError(
- "result rank must be one less than ARRAY");
- // With dim the result has rank n
- if (!dim && resultShape.size() != 1)
- return reductionOp->emitOpError("result rank must be 1");
} else {
return reductionOp->emitOpError("result must be of numerical expr type");
}
@@ -995,8 +848,6 @@ mlir::LogicalResult hlfir::DotProductOp::verify() {
llvm::ArrayRef<int64_t> rhsShape = rhsTy.getShape();
std::size_t lhsRank = lhsShape.size();
std::size_t rhsRank = rhsShape.size();
- mlir::Type lhsEleTy = lhsTy.getEleTy();
- mlir::Type rhsEleTy = rhsTy.getEleTy();
mlir::Type resultTy = getResult().getType();
if ((lhsRank != 1) || (rhsRank != 1))
@@ -1010,15 +861,6 @@ mlir::LogicalResult hlfir::DotProductOp::verify() {
(lhsSize != rhsSize))
return emitOpError("both arrays must have the same size");
- if (mlir::isa<fir::LogicalType>(lhsEleTy) !=
- mlir::isa<fir::LogicalType>(rhsEleTy))
- return emitOpError("if one array is logical, so should the other be");
-
- if (mlir::isa<fir::LogicalType>(lhsEleTy) !=
- mlir::isa<fir::LogicalType>(resultTy))
- return emitOpError("the result type should be a logical only if the "
- "argument types are logical");
-
if (!hlfir::isFortranScalarNumericalType(resultTy) &&
!mlir::isa<fir::LogicalType>(resultTy))
return emitOpError(
@@ -1051,22 +893,10 @@ mlir::LogicalResult hlfir::MatmulOp::verify() {
llvm::ArrayRef<int64_t> rhsShape = rhsTy.getShape();
std::size_t lhsRank = lhsShape.size();
std::size_t rhsRank = rhsShape.size();
- mlir::Type lhsEleTy = lhsTy.getEleTy();
- mlir::Type rhsEleTy = rhsTy.getEleTy();
- hlfir::ExprType resultTy = getResult().getType().cast<hlfir::ExprType>();
- llvm::ArrayRef<int64_t> resultShape = resultTy.getShape();
- mlir::Type resultEleTy = resultTy.getEleTy();
if (((lhsRank != 1) && (lhsRank != 2)) || ((rhsRank != 1) && (rhsRank != 2)))
return emitOpError("array must have either rank 1 or rank 2");
- if ((lhsRank == 1) && (rhsRank == 1))
- return emitOpError("at least one array must have rank 2");
-
- if (mlir::isa<fir::LogicalType>(lhsEleTy) !=
- mlir::isa<fir::LogicalType>(rhsEleTy))
- return emitOpError("if one array is logical, so should the other be");
-
int64_t lastLhsDim = lhsShape[lhsRank - 1];
int64_t firstRhsDim = rhsShape[0];
constexpr int64_t unknownExtent = fir::SequenceType::getUnknownExtent();
@@ -1075,34 +905,6 @@ mlir::LogicalResult hlfir::MatmulOp::verify() {
return emitOpError(
"the last dimension of LHS should match the first dimension of RHS");
- if (mlir::isa<fir::LogicalType>(lhsEleTy) !=
- mlir::isa<fir::LogicalType>(resultEleTy))
- return emitOpError("the result type should be a logical only if the "
- "argument types are logical");
-
- llvm::SmallVector<int64_t, 2> expectedResultShape;
- if (lhsRank == 2) {
- if (rhsRank == 2) {
- expectedResultShape.push_back(lhsShape[0]);
- expectedResultShape.push_back(rhsShape[1]);
- } else {
- // rhsRank == 1
- expectedResultShape.push_back(lhsShape[0]);
- }
- } else {
- // lhsRank == 1
- // rhsRank == 2
- expectedResultShape.push_back(rhsShape[1]);
- }
- if (resultShape.size() != expectedResultShape.size())
- return emitOpError("incorrect result shape");
- if (resultShape[0] != expectedResultShape[0] &&
- expectedResultShape[0] != unknownExtent)
- return emitOpError("incorrect result shape");
- if (resultShape.size() == 2 && resultShape[1] != expectedResultShape[1] &&
- expectedResultShape[1] != unknownExtent)
- return emitOpError("incorrect result shape");
-
return mlir::success();
}
@@ -1170,25 +972,13 @@ mlir::LogicalResult hlfir::TransposeOp::verify() {
.cast<fir::SequenceType>();
llvm::ArrayRef<int64_t> inShape = arrayTy.getShape();
std::size_t rank = inShape.size();
- mlir::Type eleTy = arrayTy.getEleTy();
hlfir::ExprType resultTy = getResult().getType().cast<hlfir::ExprType>();
llvm::ArrayRef<int64_t> resultShape = resultTy.getShape();
std::size_t resultRank = resultShape.size();
- mlir::Type resultEleTy = resultTy.getEleTy();
if (rank != 2 || resultRank != 2)
return emitOpError("input and output arrays should have rank 2");
- constexpr int64_t unknownExtent = fir::SequenceType::getUnknownExtent();
- if ((inShape[0] != resultShape[1]) && (inShape[0] != unknownExtent))
- return emitOpError("output shape does not match input array");
- if ((inShape[1] != resultShape[0]) && (inShape[1] != unknownExtent))
- return emitOpError("output shape does not match input array");
-
- if (eleTy != resultEleTy)
- return emitOpError(
- "input and output arrays should have the same element type");
-
return mlir::success();
}
@@ -1218,9 +1008,6 @@ mlir::LogicalResult hlfir::MatmulTransposeOp::verify() {
std::size_t rhsRank = rhsShape.size();
mlir::Type lhsEleTy = lhsTy.getEleTy();
mlir::Type rhsEleTy = rhsTy.getEleTy();
- hlfir::ExprType resultTy = getResult().getType().cast<hlfir::ExprType>();
- llvm::ArrayRef<int64_t> resultShape = resultTy.getShape();
- mlir::Type resultEleTy = resultTy.getEleTy();
// lhs must have rank 2 for the transpose to be valid
if ((lhsRank != 2) || ((rhsRank != 1) && (rhsRank != 2)))
@@ -1230,37 +1017,6 @@ mlir::LogicalResult hlfir::MatmulTransposeOp::verify() {
mlir::isa<fir::LogicalType>(rhsEleTy))
return emitOpError("if one array is logical, so should the other be");
- // for matmul we compare the last dimension of lhs with the first dimension of
- // rhs, but for MatmulTranspose, dimensions of lhs are inverted by the
- // transpose
- int64_t firstLhsDim = lhsShape[0];
- int64_t firstRhsDim = rhsShape[0];
- constexpr int64_t unknownExtent = fir::SequenceType::getUnknownExtent();
- if (firstLhsDim != firstRhsDim)
- if ((firstLhsDim != unknownExtent) && (firstRhsDim != unknownExtent))
- return emitOpError(
- "the first dimension of LHS should match the first dimension of RHS");
-
- if (mlir::isa<fir::LogicalType>(lhsEleTy) !=
- mlir::isa<fir::LogicalType>(resultEleTy))
- return emitOpError("the result type should be a logical only if the "
- "argument types are logical");
-
- llvm::SmallVector<int64_t, 2> expectedResultShape;
- if (rhsRank == 2) {
- expectedResultShape.push_back(lhsShape[1]);
- expectedResultShape.push_back(rhsShape[1]);
- } else {
- // rhsRank == 1
- expectedResultShape.push_back(lhsShape[1]);
- }
- if (resultShape.size() != expectedResultShape.size())
- return emitOpError("incorrect result shape");
- if (resultShape[0] != expectedResultShape[0])
- return emitOpError("incorrect result shape");
- if (resultShape.size() == 2 && resultShape[1] != expectedResultShape[1])
- return emitOpError("incorrect result shape");
-
return mlir::success();
}
diff --git a/flang/test/HLFIR/invalid.fir b/flang/test/HLFIR/invalid.fir
index 56f74d8bf29e6..251f4da300e90 100644
--- a/flang/test/HLFIR/invalid.fir
+++ b/flang/test/HLFIR/invalid.fir
@@ -296,168 +296,18 @@ func.func @bad_concat_4(%arg0: !fir.ref<!fir.char<1,30>>) {
return
}
-// -----
-func.func @bad_any1(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.any' op result must have the same element type as MASK argument}}
- %0 = hlfir.any %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> !fir.logical<8>
-}
-
-// -----
-func.func @bad_any2(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32) {
- // expected-error@+1 {{'hlfir.any' op result must have the same element type as MASK argument}}
- %0 = hlfir.any %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<?x!fir.logical<8>>
-}
-
-// -----
-func.func @bad_any3(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32){
- // expected-error@+1 {{'hlfir.any' op result rank must be one less than MASK}}
- %0 = hlfir.any %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<?x?x!fir.logical<4>>
-}
-
-// -----
-func.func @bad_any4(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32) {
- // expected-error@+1 {{'hlfir.any' op result must be an array}}
- %0 = hlfir.any %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<!fir.logical<4>>
-}
-
-// -----
-func.func @bad_any5(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.any' op result must be of logical type}}
- %0 = hlfir.any %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> i32
-}
-
-// -----
-func.func @bad_any6(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.any' op result must be of logical type}}
- %0 = hlfir.any %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> !hlfir.expr<!fir.logical<4>>
-}
-
-// -----
-func.func @bad_all1(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.all' op result must have the same element type as MASK argument}}
- %0 = hlfir.all %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> !fir.logical<8>
-}
-
-// -----
-func.func @bad_all2(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32) {
- // expected-error@+1 {{'hlfir.all' op result must have the same element type as MASK argument}}
- %0 = hlfir.all %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<?x!fir.logical<8>>
-}
-
-// -----
-func.func @bad_all3(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32){
- // expected-error@+1 {{'hlfir.all' op result rank must be one less than MASK}}
- %0 = hlfir.all %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<?x?x!fir.logical<4>>
-}
-
-// -----
-func.func @bad_all4(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32) {
- // expected-error@+1 {{'hlfir.all' op result must be an array}}
- %0 = hlfir.all %arg0 dim %arg1 : (!hlfir.expr<?x?x!fir.logical<4>>, i32) -> !hlfir.expr<!fir.logical<4>>
-}
-
-// -----
-func.func @bad_all5(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.all' op result must be of logical type}}
- %0 = hlfir.all %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> i32
-}
-
-// -----
-func.func @bad_all6(%arg0: !hlfir.expr<?x!fir.logical<4>>) {
- // expected-error@+1 {{'hlfir.all' op result must be of logical type}}
- %0 = hlfir.all %arg0 : (!hlfir.expr<?x!fir.logical<4>>) -> !hlfir.expr<!fir.logical<4>>
-}
-
-// -----
-func.func @bad_count1(%arg0: !hlfir.expr<?x?x!fir.logical<4>>, %arg1: i32) {
- // expected-error@+1 {{'hlfir.count' op result must be an array}}
- %0 = hlfir.count %arg0 dim ...
[truncated]
|
|
Thanks a lot Tom for applying the comment. I would still keep parts of those checks because we need to enforce some invariants at least to generate codes that would not itself trigger other FIR / LLVM IR verifiers (my comment was maybe a bit too anti-verifiers, sorry for that). The rank and "relaxed" element types matters to generate addressing and numerical operations that will not throw verifier errors. The length parameters and extents not so much (codegen just need to pick one variable to get the extents/length param and generate code with it). Looking at these verifiers, I think we can try just relaxing the "resultExpr.getEleTy() != numTy" to ignore length parameters for the character case, and the extent check is only emitting a warning (this I am in favor or removing altogether or to make it debug only). Maybe it will belong to type/length propagation to preserve/add some fir.convert when the propagated type would conflict with the original one (i.e. replacing a |
This is mostly a revert of the initial commit. See the diff from both for the intended changes.
|
Thanks for your feedback Jean. I've brought back the old checks, and put the more strict ones behind a a command line option (default disabled). b754c8a is mostly a revert of the initial commit. It is easiest to look at the diff after both commits are applied |
carlosgalvezp
pushed a commit
to carlosgalvezp/llvm-project
that referenced
this pull request
Feb 1, 2024
The verifiers are currently very strict: requiring intrinsic operations to be used only in cases where the Fortran standard permits the intrinsic to be used. There have now been a lot of cases where these verifiers have caused bugs in corner cases. In a recent ticket, @jeanPerier pointed out that it could be useful for future optimizations if somewhat invalid uses of these operations could be allowed in dead code. See this comment: llvm#79995 (comment) In response to all of this, I have decided to relax the intrinsic operation verifiers. The intention is now to only disallow operation uses that are likely to crash the compiler. Other checks are still available under `-strict-intrinsic-verifier`. The disadvantage of this approach is that IR can now represent intrinsic invocations which are incorrect. The lowering and implementation of these intrinsic functions is unlikely to do the right thing in all of these cases, and as they should mostly be impossible to generate using normal Fortran code, these edge cases will see very little testing, before some new optimization causes them to become more common. Fixes llvm#79995
agozillon
pushed a commit
to agozillon/llvm-project
that referenced
this pull request
Feb 5, 2024
The verifiers are currently very strict: requiring intrinsic operations to be used only in cases where the Fortran standard permits the intrinsic to be used. There have now been a lot of cases where these verifiers have caused bugs in corner cases. In a recent ticket, @jeanPerier pointed out that it could be useful for future optimizations if somewhat invalid uses of these operations could be allowed in dead code. See this comment: llvm#79995 (comment) In response to all of this, I have decided to relax the intrinsic operation verifiers. The intention is now to only disallow operation uses that are likely to crash the compiler. Other checks are still available under `-strict-intrinsic-verifier`. The disadvantage of this approach is that IR can now represent intrinsic invocations which are incorrect. The lowering and implementation of these intrinsic functions is unlikely to do the right thing in all of these cases, and as they should mostly be impossible to generate using normal Fortran code, these edge cases will see very little testing, before some new optimization causes them to become more common. Fixes llvm#79995
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.
The verifiers are currently very strict: requiring intrinsic operations to be used only in cases where the Fortran standard permits the intrinsic to be used.
There have now been a lot of cases where these verifiers have caused bugs in corner cases. In a recent ticket, @jeanPerier pointed out that it could be useful for future optimizations if somewhat invalid uses of these operations could be allowed in dead code. See this comment: #79995 (comment)
In response to all of this, I have decided to relax the intrinsic operation verifiers. The intention is now to only disallow operation uses that are likely to crash the compiler. Other checks are still available under
-strict-intrinsic-verifier.The disadvantage of this approach is that IR can now represent intrinsic invocations which are incorrect. The lowering and implementation of these intrinsic functions is unlikely to do the right thing in all of these cases, and as they should mostly be impossible to generate using normal Fortran code, these edge cases will see very little testing, before some new optimization causes them to become more common.
Fixes #79995