[PATCH] D104854: Introduce intrinsic llvm.isnan

Fri Aug 20 09:33:13 PDT 2021

spatel added a comment.

In D104854#2957582 <https://reviews.llvm.org/D104854#2957582>, @lebedev.ri wrote:

> In D104854#2957529 <https://reviews.llvm.org/D104854#2957529>, @spatel wrote:
>
>> In D104854#2957471 <https://reviews.llvm.org/D104854#2957471>, @sepavloff wrote:
>>
>>> In D104854#2957423 <https://reviews.llvm.org/D104854#2957423>, @spatel wrote:
>>>
>>>> Is it intentional that we are not canonicalizing the intrinsic call back to `fcmp uno` in the default FP environment?
>>>
>>> It is lowered to unordered comparison by default. Changing `llvm.isnan` to  `fcmp uno` somewhere in IR would make it possible to optimize out the latter if fast-math mode is on. Preserving semantics of `isnan` when fast-math is in effect was one of the goals of this change.
>>
>> I understand that the codegen was supposed to be no worse, but the difference in IR causes optimizer regressions like: 
>> https://llvm.org/PR51556
>>
>> If we want this intrinsic (and its siblings that haven't been created yet) to survive through IR, then we have to enhance IR passes to recognize the new patterns. 
>> It would be easier to do this in steps: (1) create the intrinsic only if not in the default FP env, (2) update IR analysis/passes to recognize the intrinsic, (3) create the intrinsic in the default FP env with no FMF, (4) create the intrinsic always.
>
> +1, but right now i'm not sold on the behavior of not optimizing away NaN checks in no-NaN's mode.
> At least that part should be reconciled now. It *might* be an improvement, but it caters to expectations
> of one group while catering away from the documentation and existing expectations of other groups.
> This shouldn't be decided in a review, it should be driven by an RFC.

Agree. I think a revert followed by RFC to make sure there is consensus on semantics is needed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104854