Add a semantics test for the intrinsic function image_status. Add
a check and restriction on the image argument in image_status,
ensuring that it is a positive value. Add same check on the
size argument of the intrinsic ishftc. Add another check on
the shift argument of ishftc, ensuring that it is less than or
equal to the size argument. Add a short semantics test checking
these restrictions in ishftc function calls.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Thanks, it is great to be able to provide more compile time feedback. I think KindCode is not the right place to encode and complain about illegal argument values when they can be known and checked at compile time.
@klausler, would adding a new field with that purpose to IntrinsicDummyArgument make sense to you ?
@ktras , did you identify ISFHTC and IMAGE_STATUS intrinsic as being the only one having this kind of constraint ?
| flang/lib/Evaluate/intrinsics.cpp | ||
|---|---|---|
| 87 | What about the KIND constraints of such kindCode ? The KindCode enum main goal is to check for constraints about the KIND of the arguments, so it may not be the right place to encode such constraint. I think the cleaner way would be to create a new enum to encode value domain constraints and stick it in IntrinsicDummyArgument. But I am not sure complexifying the infrastructure/table it is worth it if they are few intrinsic argument that have such constraints, maybe custom checks make more sense. | |
| 1469 | You should be able to merge these three lines into if (auto val{ToInt64(arg->UnwrapExpr())}) { ..., ToInt64 can take optional arguments, and returns nullopt if the arg is not a constant. Also, you might be aware of that, but the argument may be arrays, in which case ToInt64 will fail (it cannot make a single value out of an array), and the check will not be applied on things like: subroutine bad(i, j) integer :: i(2), j(2) print *, ishftc(i, j, [-1, 1]) end subroutine | |
Comment Actions
No. That struct is too complicated already. A custom check would be better, as is done for several other cases. Also, lowering will need to worry about the SIZE= argument to ISHFTC being an absent incoming dummy argument.
Comment Actions
Move the check and error creation for a non-positive value passed to the image argument of image_status and the size argument of ishftc to ApplySpecificChecks(). Add a check on the shift argument of ishftc, ensuring that it is less than or equal to the size argument and add to the ishftc semantics test.
Comment Actions
@jeanPerier and @klausler Thank you for your feedback.
I have moved the check for non positive values to ApplySpecificChecks(). Is this an appropriate place for the type of check I am trying to add?
@jeanPerier The image argument of image_status and the size argument of ishftc were the only arguments to intrinsic functions that I could find that had that type of restriction, requiring them to be greater than 0. Though it is possible I missed
something. Also, yes, I knew that you could pass arrays but I definitely spaced on that when writing the new error check, so thank you for pointing that out to me. I spent some time trying to account for the case when the argument is an array, but was unable to find a way to do so. I believe the class I am dealing with when I unwrap the expression inside of the argument is Expr<SomeType>. I am not sure how to ask this question, but if I have an instance of Expr<SomeType> and I know that it is an array (through checking the rank of the argument) , is there a way to unpack the Expr<SomeType> so that I have something like an array of Expr<SomeType>, so that I can iterate over the array and do the check for a non-positive value? This is what I was trying to track down and I couldn't find. I would appreciate any advice or pointing towards portions of code that might be helpful for me to look at, thanks!
| flang/lib/Evaluate/intrinsics.cpp | ||
|---|---|---|
| 2389 | It didn't cross my mind to, but I can definitely try to come up with an error message that includes the bad value. Thanks. | |
Comment Actions
I am not sure how to ask this question, but if I have an instance of Expr<SomeType> and I know that it is an array (through checking the rank of the argument) , is there a way to unpack the Expr<SomeType> so that I have something like an array of Expr<SomeType>, so that I can iterate over the array and do the check for a non-positive value? This is what I was trying to track down and I couldn't find. I would appreciate any advice or pointing towards portions of code that might be helpful for me to look at, thanks!
An Expr<SomeType> that is an array is not represented as an array of Expr<SomeType> in general because it may not be feasible: e.g. retrun_some_array() transformational call cannot be split in an array of Expr<SomeType> elements, this needs a lower level representation that will evaluate retrun_some_array in some memory storage first, and this happens only in lowering (and would not matter here, since it is very unlikely we ever could execute the call here at compile time to check for negative elements).
However, there are two kinds of array expressions where you have some opportunities to do some checks: array constants, and array constructors, for those expressions, you can visit all (or some for the array constructor that may also contain array element.
Expr<SomeType>
|
wraps (variant, here you know you should have integers only, so you can use if (const auto *intExpr{UnwrapExpr<Expr<SomeInteger>>(expr)}) { /*your code unwrapping the next level*/}
|
\/
Expr<SomeKind<Category>>>
|
wraps (variant, here you can have all kinds, so you have to use std::visit([&](const auto kindExpr&) { /*your code unwrapping the next level*/ }, intExpr->u);
|
\/
Expr<Type<Category, Kind>>>
|
wraps------------------------------------------------------- or -----wraps ------- or some other nodes you probably can ignore here. Use std::visit(common::visitor ..., kindExpr.u)
| |
\/ \/
Constant<Type<Category, Kind>>> ArrayConstructor<Type<Category, Kind>>>
| |
\/ \/
You can use .values() You can use for(const auto& arrayCtorValues : arrayConstructor) {...} to loop
to visit the Scalar<Type<Cat, Kind>> elements. through the ArrayConstructorValue that may be expressions, or ImpliedDo
They have a IsNegative() method. You could only visit the expressions, calling your scalar check again.
`Now, as you see this requires some visit effort, and you can go very far if you want to check implied-dos to find a negative constant element somwhere... I think you should simply focus on dealing with the Constant<T> case that is much simpler, and skip the ArrayConstructor<> case. The check you are adding is bonus, having it working on scalar and array constants seems enough to me.
Comment Actions
Added a check for non positive values if an array is passed to the image argument of image_status or the size argument of ishftc.
Comment Actions
@klausler I did try to come up with an error message that included the actual value that was passed, but I couldn't come up with any message that was concise or that I liked better than the current error message. I am of course open to suggestions. And thank you for your feedback on the better way to access the object inside of an std::optional.
@jeanPerier Thank you for your helpful suggestions on how to unpack the Expr<SomeType> if it was an array. I have unpacked it now and was able to provide an error message if a non positive value is included in the array. There might be a more elegant way to deal with all of the different kinds, but this is what I have come up with for now.
| flang/lib/Evaluate/intrinsics.cpp | ||
|---|---|---|
| 2353 | Please don't copy-and-paste code for each currently supported kind of integer. Use an abstraction that covers all of them. | |
| 2390 | "'%s=' argument for intrinsic function '%s' must be a positive value, but is %jd"_err_en_US, argName, procName, static_cast<std::intmax_t>(*val) | |
| flang/lib/Evaluate/intrinsics.cpp | ||
|---|---|---|
| 2353 | common::visit([&](const auto &kindExpr) {
using IntType = std::decay_t<decltype(kindExpr)>::Result;
if (const auto *constArray{UnwrapConstantValue<IntType>(kindExpr)}) {
ok = CheckArrayForNonPositive(constArray, context, arg, procName, argName);
}
}, intExpr->u);UnwrapConstantValue() will also work through parentheses. (just a suggestion; not tested) | |
Comment Actions
Replaced the hardcoded dealing with the different kind codes in Expr<SomeInteger> with dealing with them through an abstraction.
Comment Actions
@klausler Thank you for the suggested usage of std::decay_t. I didn't want to deal with the kind codes through hardcoding, so thank you for helping me see how could I deal with them through an abstraction in that context. Also, thank you for the suggested error message. That is much more concise than the versions I came up with!
Comment Actions
@klausler Thanks for the review.
@jeanPerier Since you gave feedback earlier, would you like me to wait on your review before I push to main? I was thinking about pushing by the end of the workday today, if I didn't get any more feedback, but please let me know if you would like me to wait.
What about the KIND constraints of such kindCode ?
The KindCode enum main goal is to check for constraints about the KIND of the arguments, so it may not be the right place to encode such constraint.
I think the cleaner way would be to create a new enum to encode value domain constraints and stick it in IntrinsicDummyArgument. But I am not sure complexifying the infrastructure/table it is worth it if they are few intrinsic argument that have such constraints, maybe custom checks make more sense.