[PATCH] D128009: [flang] Add semantics test for image_status and add a check

[Jean Perier via Phabricator via llvm-commits]

jeanPerier added a comment.

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 ?



================
Comment at: flang/lib/Evaluate/intrinsics.cpp:86
     dimArg, // this argument is DIM=
+    positiveInt, // integer arguments that must be positive
     likeMultiply, // for DOT_PRODUCT and MATMUL
----------------
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.




================
Comment at: flang/lib/Evaluate/intrinsics.cpp:1470
+        if (IsConstantExpr(*expr)) {
+          if (auto val{ToInt64(*expr)}) {
+            if (val <= 0) {
----------------
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
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128009/new/

https://reviews.llvm.org/D128009