[flang-commits] [PATCH] D129296: Lower mask intrinsics

[Jean Perier via Phabricator via flang-commits]

jeanPerier added a comment.

Thanks for working on this.

Where do you get the "If I < 0 or I > BIT_SIZE(KIND), the result will always be 0." requirement ? My understanding from the standard it is non conforming if  I < 0 or I > BIT_SIZE(KIND), and so there is no specified behavior.

I checked with a few compilers, but it seems only xlf does this, and ifort for the <0 case, otherwise, gfortran, nag and ifort (for the  I > BIT_SIZE(KIND) case) returns the same but non zero results.
I also noticed that f18 semantics folds the `I > BIT_SIZE(KIND)` to -1 (all other compilers, except xlf, raise a compile time error in these cases).

Anyway, I am raising this because I do not know if the extra ops are worth it. Is it really safer to return zero in these case, or is it an extension some program expects ? ON the other hand, I can understand that zero mask are probably more likely to generate results that a programmer would quickly question, and I am not sure what is the kind of perf hit adding these instructions would cause. A better (future, no need to do that here), would probably to add a way to have runtime checks with a fatal error for this kind of situation under some specific flags.

Please do not abandon the behavior of your patch right away though, I am not expert enough in the usages of MASKL./MASKR to have a strong opinion.

@vdonaldson  or @klausler, do you think f18 need to deal with the "If I < 0 or I > BIT_SIZE(KIND)" case in a special way at runtime ?



================
Comment at: flang/lib/Lower/IntrinsicCall.cpp:3193
+      loc, resultType, resultType.getIntOrFloatBitWidth());
+  if (args[1]) {
+    mlir::Value eight = builder.createIntegerConstant(loc, resultType, 8);
----------------
I am surprised you have to do that here. the KIND argument, if present, must already be represented in the resultType that semantics selected for the intrinsic. So I think you should be able to remove this case (otherwise, this would indicate some bug with resultType).


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

https://reviews.llvm.org/D129296