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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

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

> It is the middle end's responsibility to make only transformations that keep FP semantics. Replacement `is_fpclass` -> `fcmp` changes the semantics and can produce incorrect programs.

It doesn't change the semantics because the FP exception that the machine fcmp may end up raising can be assumed to be invisible based on the IR semantics. An unconstrained fcmp does not have observable FP exceptions. For any regular non-constrained operation, you simply never need to be concerned with floating point exceptions. They can't be observed or relied on.

> In D137811#4083368 <https://reviews.llvm.org/D137811#4083368>, @kpn wrote:
>
>> Ok, true, it takes executing code to change the floating point environment. Yes, there would be inline assembly or a function call to change the FP environment. LLVM would see that code because there would be IR for it. All true.
>>
>> But LLVM wouldn't know what the inline assembly was doing. It wouldn't recognize the function call and thus wouldn't know what it was doing. LLVM would not know the floating point environment had changed. Rephrased, the change in the floating point environment would, to LLVM, be invisible. That's the "invisible" I was referring to earlier.
>
> Ok, I see what is "invisible". Do you think inline assembly should be decorated as def/use of FP environment similar to function calls as is done in D111433 <https://reviews.llvm.org/D111433> and D139549 <https://reviews.llvm.org/D139549>?
>
>> It's true that a blind replacement of is_fpclass with fcmp would be incorrect _if_ the floating point environment is not the default environment. But if we are in the default FP environment we can assume that any CPU trap will be hidden from us by the OS and therefore is not a part of our discussion of IR correctness.
>
> 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.

This code didn't actually manipulate the floating point mode, just made it legal to do so for the scope of the pragma. If you were to insert code to modify the mode to enable FP exceptions inside func2, it would be illegal because you did not restore the mode before calling into code outside of the pragma scope. FENV_ACCESS doesn't require the compiler to fixup the floating point mode for you before calling into other code.


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

https://reviews.llvm.org/D137811