[PATCH] D106362: [FPEnv][InstSimplify] Enable more folds for constrained fadd

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D106362#3032895 <https://reviews.llvm.org/D106362#3032895>, @sepavloff wrote:

> In D106362#3031370 <https://reviews.llvm.org/D106362#3031370>, @kpn wrote:
>
>> In D106362#3031030 <https://reviews.llvm.org/D106362#3031030>, @sepavloff wrote:
>>
>>> In D106362#3030871 <https://reviews.llvm.org/D106362#3030871>, @kpn wrote:
>>>
>>>> In D106362#3030856 <https://reviews.llvm.org/D106362#3030856>, @sepavloff wrote:
>>>>
>>>>> In D106362#3030790 <https://reviews.llvm.org/D106362#3030790>, @kpn wrote:
>>>>>
>>>>>> Changing the way the existing transforms check for FMF.noNaNs() sounds like a different ticket that needs to be done before this one can progress. Unless I misunderstood?
>>>>>
>>>>> I know nothing about such change.
>>>>
>>>> Your proposed changes around SNaN will affect the non-constrained cases.
>>>
>>> I think the optimization `fadd X, -0 ==> X` in general case (not `FMF.noNaNs()`) is incorrect. The obtained values must be identical to what would produce hardware, otherwise it is not an optimization. If you think that such change deserves a separate patch, no problem. It seems to me that this patch could establish correct folding even if it changes non-constrained operation as well.
>>
>> I really don't think we should be changing how LLVM handles the ebIgnore cases. LLVM currently treats SNaN and QNaN as the same except where it doesn't, and I believe we should keep that behavior unchanged when ignoring exceptions. At the very least we shouldn't be disabling any currently enabled optimizations when ignoring exceptions. @lebedev.ri or @jcranmer ?

I agree.

> It is actually a bug. None of the operation in IEEE-754 compatible implementation may produce SNaN. If some code that would produce QNaN in runtime produces SNaN due to constant folding, such optimization is invalid. A user might use the expression `x + 0.0` just to convert SNaN to QNaN, it won't work in the case of such optimization.



>> Our closest approximation to a strict IEEE-754 mode currently is with ebStrict / "fpexcept.strict" usage, but that isn't documented as being strictly IEEE-754 compliant. If someone wants to go through and add a strict -754 mode at some point then they can. But that's a different project.
>
> Most hardware FP implementations adhere IEEE-754. As LLVM IR is by design target-independent, non-IEEE-754 implementation hardly is interesting. Also, `ebIgnore` is only an optimization hint, it should not change the semantics of the `add` operation.

`fadd X, -0 ==> X` is *NOT* a miscompile, at least given the current LLVM IR semantics: https://alive2.llvm.org/ce/z/TuTiSQ
I would personally strongly suggest to not reason about semantics via hand-waving, but to actually model them in alive2, if it isn't already.
Honestly, i'm quite worried that this is repeating the same approach as in isnan threads.
Some might interpret it as being dismissive/intentionally ignoring documented semantics.


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

https://reviews.llvm.org/D106362