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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added a comment.

In D137811#4066479 <https://reviews.llvm.org/D137811#4066479>, @arsenm wrote:

> In D137811#4065720 <https://reviews.llvm.org/D137811#4065720>, @arsenm wrote:
>
>>> But they have different behavior if `x` is signaling NaN. Although the replacement is proposed for strictfp functions only, inlining may require conversion to a form that uses constrained intrinsics. The resulting code would have different behavior than original.
>>
>> A transformation that’s valid for !strictfp functions cannot be invalid because of the potential for being inlined into a strictfp function. If this isn’t possible, there is a representational issue the inliner would need to account for.
>
> Thinking about this more, I think any situation where this could be an issue would be malformed to begin with. You had a piece of code that was expecting exceptions to be enabled calling into code where exceptions were disabled without turning exceptions off

If exceptions were turned off, the call was made, and exceptions were turned back on then there would be no correctness issue. Inlining the called function wouldn't change that. The toggling of the exception enablement would be invisible to LLVM. Thus we can't categorically say that a strictfp function calling a !strictfp function is malformed.

The constrained intrinsics that specify the default rounding and exceptions disabled are supposed to behave the same as the normal instructions. They're supposed to have the exact same meaning and behavior. So a conversion of isnan(x) to (x == x) for the constrained intrinsics in the default environment is just as correct as the instructions that do the same thing but aren't constrained intrinsics. Thus inlining of a !strictfp function into a strictfp function with the conversion of FP instructions into constrained intrinsics can't introduce any new correctness issues.


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

https://reviews.llvm.org/D137811