Skip to content

[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 13 commits into from
Jul 2, 2024
240 changes: 237 additions & 3 deletions flang/lib/Evaluate/intrinsics-library.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Copy link
Contributor

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>, with std::nullopt signifying the last argument. You can then access the argument via .value_or(args.size()-1).

Copy link
Contributor Author

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 using args.back(), which seems pretty clear to me.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lb and ub are integers; why emit decimal points?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The values in Fortran are REAL, even though the values that the compiler deals with are int.

}
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.
Expand All @@ -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) {
Expand All @@ -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());
};
}
Expand Down
28 changes: 23 additions & 5 deletions flang/test/Evaluate/folding04.f90
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,42 @@ module real_tests
!WARN: warning: division by zero
real(4), parameter :: r4_ninf = -1._4/0._4

!WARN: warning: invalid argument on evaluation of intrinsic function or operation
!WARN: warning: argument is out of range [-1., 1.]
real(4), parameter :: nan_r4_acos1 = acos(1.1)
TEST_ISNAN(nan_r4_acos1)
!WARN: warning: invalid argument on evaluation of intrinsic function or operation
!WARN: warning: argument is out of range [-1., 1.]
real(4), parameter :: nan_r4_acos2 = acos(r4_pmax)
TEST_ISNAN(nan_r4_acos2)
!WARN: warning: invalid argument on evaluation of intrinsic function or operation
!WARN: warning: argument is out of range [-1., 1.]
real(4), parameter :: nan_r4_acos3 = acos(r4_nmax)
TEST_ISNAN(nan_r4_acos3)
!WARN: warning: invalid argument on evaluation of intrinsic function or operation
!WARN: warning: argument is out of range [-1., 1.]
real(4), parameter :: nan_r4_acos4 = acos(r4_ninf)
TEST_ISNAN(nan_r4_acos4)
!WARN: warning: invalid argument on evaluation of intrinsic function or operation
!WARN: warning: argument is out of range [-1., 1.]
real(4), parameter :: nan_r4_acos5 = acos(r4_pinf)
TEST_ISNAN(nan_r4_acos5)
!WARN: warning: argument is out of range [-1., 1.]
real(8), parameter :: nan_r8_dasin1 = dasin(-1.1_8)
TEST_ISNAN(nan_r8_dasin1)
!WARN: warning: complex argument must be different from zero
complex(4), parameter :: c4_clog1 = clog((0., 0.))
!WARN: warning: MOD: P argument is zero
real(4), parameter :: nan_r4_mod = mod(3.5, 0.)
TEST_ISNAN(nan_r4_mod)
real(4), parameter :: ok_r4_gamma = gamma(-1.1)
!WARN: warning: argument must not be a negative integer or zero
real(4), parameter :: r4_gamma1 = gamma(0.)
!WARN: warning: argument must not be a negative integer or zero
real(4), parameter :: r4_gamma2 = gamma(-1.)
real(4), parameter :: ok_r4_log_gamma = log_gamma(-2.001)
!WARN: warning: argument must not be a negative integer or zero
real(4), parameter :: r4_log_gamma1 = log_gamma(0.)
!WARN: warning: argument must not be a negative integer or zero
real(4), parameter :: r4_log_gamma2 = log_gamma(-100001.)
!WARN: warning: 'x' and 'y' arguments must not be both zero
real(4), parameter :: r4_atan2 = atan2(0., 0.)

!WARN: warning: overflow on evaluation of intrinsic function or operation
logical, parameter :: test_exp_overflow = exp(256._4).EQ.r4_pinf
contains
Expand Down