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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4908
+      (!RoundingModeMayBe(Rounding, RoundingMode::TowardNegative) &&
+       (ExBehavior != fp::ebStrict || FMF.noNaNs())))
+    if (match(Op1, m_NegZeroFP()))
----------------
spatel wrote:
> sepavloff wrote:
> > kpn wrote:
> > > sepavloff wrote:
> > > > Even if `ExBehavior != fp::ebStrict` this transformation is invalid if `X` is SNaN, the latter must be converted to QNaN.
> > > > 
> > > > 
> > > Can I put this in a different patch? We have the same issue for constrained and non-constrained cases, and we don't have a good way to distinguish them -- nor should we.
> > > 
> > > Most of the required code is present in APFloat, but not all. Would it be OK for me to add the needed bits to APFloat and use them from InstSimplify in a different ticket?
> > I am not sure I understand what you are going to put into another patch. The transformation:
> > 
> >     fadd X, -0 ==> X
> > 
> > is valid only if `X != SNaN`, otherwise we have:
> > 
> >     fadd SNaN, -0 ==> QNaN
> > 
> > It does not depend on whether FP environment is default or not, the code before your changes was already incorrect. Such transformation is valid if the operation has flag `nnan`, but not in general case. Code in APFloat hardly can help, as constant folding is made previously, if `X` is a constant here, it means it cannot be folded.
> IIUC, you are saying SNaN vs. QNaN is more than just a part of the exception state. But that's not how I interpret the current LangRef:
> https://llvm.org/docs/LangRef.html#floating-point-environment
> 
> ...and that's why *none* of the transforms here are intentionally SNaN preserving/clearing. 
> 
> If we are going to change the behavior of the default FP env, the LangRef must be updated to make that clear. I don't see the motivation yet.
That's true for default FP environment. A difference between SNaN and QNaN is that operations on SNaN raise invalid exception. In default FP environment exceptions are ignored so SNaN and QNaN behave identically.

But this code works in non-default FP environment as well. `fadd` here designates both regular IR node used in default environment as well as its constrained counterpart. So SNaN here must be handled more carefully.


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

https://reviews.llvm.org/D106362