[PATCH] D137811: InstCombine: Perform basic isnan combines on llvm.is.fpclass

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added a comment.

In D137811#4084663 <https://reviews.llvm.org/D137811#4084663>, @sepavloff wrote:

> What about such code?
>
>   int get_code(float x) {
>     return isnan(x) ? 1 : 2;
>   }
>   void func1(float x) {
>     int code1 = get_code(x);
>     ...
>   }
>   #pragma STDC FENV_ACCESS ON
>   void func2(float x) {
>     int code1 = get_code(x);
>     ...
>   }
>
> Acoording to C standard it must work as intended. If compiler replaces `isnan` with comparison, the code would be broken.

>From the C standard (7.6.1p2 from draft N3054):

> (When execution passes from a part of the program translated with `FENV_ACCESS` "off" to a part translated with `FENV_ACCESS` "on", the state of the floating-point status flags is unspecified and the floating-point control modes have their default settings.)



> 263. The purpose of the `FENV_ACCESS` pragma is to allow certain optimizations that could subvert flag tests and mode changes (e.g., global common subexpression elimination, code motion, and constant folding). In general, if the state of `FENV_ACCESS` is "off", the translator can assume that the flags are not tested, and that default modes are in effect, except where specified otherwise by an `FENV_ROUND` pragma.

On a strict reading of the standard, on transition from `get_code` (which has `FENV_ACCESS OFF`) back to `func2`, the FP flags are unspecified, which means it is legal to transform the non-flag-setting `isnan` into flag-setting `fcmp`. There is a slight wording mismatch, as `FENV_ACCESS` is about //testing//, not //setting// flags, but the footnote seems to indicate that the correct reading is that `FENV_ACCESS` is necessary if you want guarantees about flags being set or not set.

If it would help, I could contact the CFP study group to see what they think is the correct reading of the standard.


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

https://reviews.llvm.org/D137811