-
Notifications
You must be signed in to change notification settings - Fork 14.2k
[flang] Improve warnings for invalid arguments when folding host runtime #96807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
01d1340
[flang] Improve warnings for invalid arguments when folding host runtime
psteinfeld 59be62c
Just trying to figure out why the Windows build is failing.
psteinfeld b3a822d
Second test.
psteinfeld 5513c55
Third test.
psteinfeld 44bc833
Fourth test.
psteinfeld 3ef0fa7
Fifth test.
psteinfeld b04d787
Sixth test.
psteinfeld 704fbc6
Seventh test.
psteinfeld 3f72b69
Eighth test.
psteinfeld 49853a4
Ninth test.
psteinfeld 83da7e6
Tenth test.
psteinfeld 95933cd
Responding to Peter's comments.
psteinfeld ce70597
Marking some of the new messages as warnings.
psteinfeld File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -633,10 +633,243 @@ static DynamicType BiggerType(DynamicType type) { | |
return type; | ||
} | ||
|
||
/// Structure to register intrinsic argument checks that must be performed. | ||
using ArgumentVerifierFunc = bool (*)( | ||
const std::vector<Expr<SomeType>> &, FoldingContext &); | ||
struct ArgumentVerifier { | ||
using Key = std::string_view; | ||
// Needed for implicit compare with keys. | ||
constexpr operator Key() const { return key; } | ||
Key key; | ||
ArgumentVerifierFunc verifier; | ||
}; | ||
|
||
static constexpr int lastArg{-1}; | ||
static constexpr int firstArg{0}; | ||
|
||
static const Expr<SomeType> &GetArg( | ||
int position, const std::vector<Expr<SomeType>> &args) { | ||
if (position == lastArg) { | ||
CHECK(!args.empty()); | ||
return args.back(); | ||
} | ||
CHECK(position >= 0 && static_cast<std::size_t>(position) < args.size()); | ||
return args[position]; | ||
} | ||
|
||
template <typename T> | ||
static bool IsInRange(const Expr<T> &expr, int lb, int ub) { | ||
if (auto scalar{GetScalarConstantValue<T>(expr)}) { | ||
auto lbValue{Scalar<T>::FromInteger(value::Integer<8>{lb}).value}; | ||
auto ubValue{Scalar<T>::FromInteger(value::Integer<8>{ub}).value}; | ||
return Satisfies(RelationalOperator::LE, lbValue.Compare(*scalar)) && | ||
Satisfies(RelationalOperator::LE, scalar->Compare(ubValue)); | ||
} | ||
return true; | ||
} | ||
|
||
/// Verify that the argument in an intrinsic call belongs to [lb, ub] if is | ||
/// real. | ||
template <int lb, int ub> | ||
static bool VerifyInRangeIfReal( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someReal{ | ||
std::get_if<Expr<SomeReal>>(&GetArg(firstArg, args).u)}) { | ||
bool isInRange{ | ||
std::visit([&](const auto &x) -> bool { return IsInRange(x, lb, ub); }, | ||
someReal->u)}; | ||
if (!isInRange) { | ||
context.messages().Say( | ||
"argument is out of range [%d., %d.]"_warn_en_US, lb, ub); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The values in Fortran are |
||
} | ||
return isInRange; | ||
} | ||
return true; | ||
} | ||
|
||
template <int argPosition, const char *argName> | ||
static bool VerifyStrictlyPositiveIfReal( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someReal = | ||
std::get_if<Expr<SomeReal>>(&GetArg(argPosition, args).u)) { | ||
const bool isStrictlyPositive{std::visit( | ||
[&](const auto &x) -> bool { | ||
using T = typename std::decay_t<decltype(x)>::Result; | ||
auto scalar{GetScalarConstantValue<T>(x)}; | ||
return Satisfies( | ||
RelationalOperator::LT, Scalar<T>{}.Compare(*scalar)); | ||
}, | ||
someReal->u)}; | ||
if (!isStrictlyPositive) { | ||
context.messages().Say( | ||
"argument '%s' must be strictly positive"_warn_en_US, argName); | ||
} | ||
return isStrictlyPositive; | ||
} | ||
return true; | ||
} | ||
|
||
/// Verify that an intrinsic call argument is not zero if it is real. | ||
template <int argPosition, const char *argName> | ||
static bool VerifyNotZeroIfReal( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someReal = | ||
std::get_if<Expr<SomeReal>>(&GetArg(argPosition, args).u)) { | ||
const bool isNotZero{std::visit( | ||
[&](const auto &x) -> bool { | ||
using T = typename std::decay_t<decltype(x)>::Result; | ||
auto scalar{GetScalarConstantValue<T>(x)}; | ||
return !scalar || !scalar->IsZero(); | ||
}, | ||
someReal->u)}; | ||
if (!isNotZero) { | ||
context.messages().Say( | ||
"argument '%s' must be different from zero"_warn_en_US, argName); | ||
} | ||
return isNotZero; | ||
} | ||
return true; | ||
} | ||
|
||
/// Verify that the argument in an intrinsic call is not zero if is complex. | ||
static bool VerifyNotZeroIfComplex( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someComplex = | ||
std::get_if<Expr<SomeComplex>>(&GetArg(firstArg, args).u)) { | ||
const bool isNotZero{std::visit( | ||
[&](const auto &z) -> bool { | ||
using T = typename std::decay_t<decltype(z)>::Result; | ||
auto scalar{GetScalarConstantValue<T>(z)}; | ||
return !scalar || !scalar->IsZero(); | ||
}, | ||
someComplex->u)}; | ||
if (!isNotZero) { | ||
context.messages().Say( | ||
"complex argument must be different from zero"_warn_en_US); | ||
} | ||
return isNotZero; | ||
} | ||
return true; | ||
} | ||
|
||
// Verify that the argument in an intrinsic call is not zero and not a negative | ||
// integer. | ||
static bool VerifyGammaLikeArgument( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someReal = | ||
std::get_if<Expr<SomeReal>>(&GetArg(firstArg, args).u)) { | ||
const bool isValid{std::visit( | ||
[&](const auto &x) -> bool { | ||
using T = typename std::decay_t<decltype(x)>::Result; | ||
auto scalar{GetScalarConstantValue<T>(x)}; | ||
if (scalar) { | ||
return !scalar->IsZero() && | ||
!(scalar->IsNegative() && | ||
scalar->ToWholeNumber().value == scalar); | ||
} | ||
return true; | ||
}, | ||
someReal->u)}; | ||
if (!isValid) { | ||
context.messages().Say( | ||
"argument must not be a negative integer or zero"_warn_en_US); | ||
} | ||
return isValid; | ||
} | ||
return true; | ||
} | ||
|
||
// Verify that two real arguments are not both zero. | ||
static bool VerifyAtan2LikeArguments( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
if (const auto *someReal = | ||
std::get_if<Expr<SomeReal>>(&GetArg(firstArg, args).u)) { | ||
const bool isValid{std::visit( | ||
[&](const auto &typedExpr) -> bool { | ||
using T = typename std::decay_t<decltype(typedExpr)>::Result; | ||
auto x{GetScalarConstantValue<T>(typedExpr)}; | ||
auto y{GetScalarConstantValue<T>(GetArg(lastArg, args))}; | ||
if (x && y) { | ||
return !(x->IsZero() && y->IsZero()); | ||
} | ||
return true; | ||
}, | ||
someReal->u)}; | ||
if (!isValid) { | ||
context.messages().Say( | ||
"'x' and 'y' arguments must not be both zero"_warn_en_US); | ||
} | ||
return isValid; | ||
} | ||
return true; | ||
} | ||
|
||
template <ArgumentVerifierFunc... F> | ||
static bool CombineVerifiers( | ||
const std::vector<Expr<SomeType>> &args, FoldingContext &context) { | ||
return (... & F(args, context)); | ||
} | ||
|
||
/// Define argument names to be used error messages when the intrinsic have | ||
/// several arguments. | ||
static constexpr char xName[]{"x"}; | ||
static constexpr char pName[]{"p"}; | ||
|
||
/// Register argument verifiers for all intrinsics folded with runtime. | ||
static constexpr ArgumentVerifier intrinsicArgumentVerifiers[]{ | ||
{"acos", VerifyInRangeIfReal<-1, 1>}, | ||
{"asin", VerifyInRangeIfReal<-1, 1>}, | ||
{"atan2", VerifyAtan2LikeArguments}, | ||
{"bessel_y0", VerifyStrictlyPositiveIfReal<firstArg, xName>}, | ||
{"bessel_y1", VerifyStrictlyPositiveIfReal<firstArg, xName>}, | ||
{"bessel_yn", VerifyStrictlyPositiveIfReal<lastArg, xName>}, | ||
{"gamma", VerifyGammaLikeArgument}, | ||
{"log", | ||
CombineVerifiers<VerifyStrictlyPositiveIfReal<firstArg, xName>, | ||
VerifyNotZeroIfComplex>}, | ||
{"log10", VerifyStrictlyPositiveIfReal<firstArg, xName>}, | ||
{"log_gamma", VerifyGammaLikeArgument}, | ||
{"mod", VerifyNotZeroIfReal<lastArg, pName>}, | ||
}; | ||
|
||
const ArgumentVerifierFunc *findVerifier(const std::string &intrinsicName) { | ||
static constexpr Fortran::common::StaticMultimapView<ArgumentVerifier> | ||
verifiers(intrinsicArgumentVerifiers); | ||
static_assert(verifiers.Verify(), "map must be sorted"); | ||
auto range{verifiers.equal_range(intrinsicName)}; | ||
if (range.first != range.second) { | ||
return &range.first->verifier; | ||
} | ||
return nullptr; | ||
} | ||
|
||
/// Ensure argument verifiers, if any, are run before calling the runtime | ||
/// wrapper to fold an intrinsic. | ||
static HostRuntimeWrapper AddArgumentVerifierIfAny( | ||
const std::string &intrinsicName, const HostRuntimeFunction &hostFunction) { | ||
if (const auto *verifier{findVerifier(intrinsicName)}) { | ||
const HostRuntimeFunction *hostFunctionPtr = &hostFunction; | ||
return [hostFunctionPtr, verifier]( | ||
FoldingContext &context, std::vector<Expr<SomeType>> &&args) { | ||
const bool validArguments{(*verifier)(args, context)}; | ||
if (!validArguments) { | ||
// Silence fp signal warnings since a more detailed warning about | ||
// invalid arguments was already emitted. | ||
parser::Messages localBuffer; | ||
parser::ContextualMessages localMessages{&localBuffer}; | ||
FoldingContext localContext{context, localMessages}; | ||
return hostFunctionPtr->folder(localContext, std::move(args)); | ||
} | ||
return hostFunctionPtr->folder(context, std::move(args)); | ||
}; | ||
} | ||
return hostFunction.folder; | ||
} | ||
|
||
std::optional<HostRuntimeWrapper> GetHostRuntimeWrapper(const std::string &name, | ||
DynamicType resultType, const std::vector<DynamicType> &argTypes) { | ||
if (const auto *hostFunction{SearchHostRuntime(name, resultType, argTypes)}) { | ||
return hostFunction->folder; | ||
return AddArgumentVerifierIfAny(name, *hostFunction); | ||
} | ||
// If no exact match, search with "bigger" types and insert type | ||
// conversions around the folder. | ||
|
@@ -647,7 +880,8 @@ std::optional<HostRuntimeWrapper> GetHostRuntimeWrapper(const std::string &name, | |
} | ||
if (const auto *hostFunction{ | ||
SearchHostRuntime(name, biggerResultType, biggerArgTypes)}) { | ||
return [hostFunction, resultType]( | ||
auto hostFolderWithChecks{AddArgumentVerifierIfAny(name, *hostFunction)}; | ||
return [hostFunction, resultType, hostFolderWithChecks]( | ||
FoldingContext &context, std::vector<Expr<SomeType>> &&args) { | ||
auto nArgs{args.size()}; | ||
for (size_t i{0}; i < nArgs; ++i) { | ||
|
@@ -657,7 +891,7 @@ std::optional<HostRuntimeWrapper> GetHostRuntimeWrapper(const std::string &name, | |
} | ||
return Fold(context, | ||
ConvertToType( | ||
resultType, hostFunction->folder(context, std::move(args))) | ||
resultType, hostFolderWithChecks(context, std::move(args))) | ||
.value()); | ||
}; | ||
} | ||
|
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using magic values, consider using
std::optional<int>
, withstd::nullopt
signifying the last argument. You can then access the argument via.value_or(args.size()-1)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the advantage of this. It seems like
std::nullopt
just becomes another kind of magic value. Currently, the code accesses the last argument usingargs.back()
, which seems pretty clear to me.