[PATCH] D103169: [FPEnv][InstSimplify] Constrained FP support for NaN

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4821
 
-    if (IsUndef || IsNan)
+    if ((IsUndef || IsNan) && ExBehavior != fp::ebStrict)
       return propagateNaN(cast<Constant>(V));
----------------
kpn wrote:
> sepavloff wrote:
> > Even if the exception behavior is `ebStrict` NaN propagation is not allowed only for signaling NaN, quiet NaN propagates quietly, without setting FP exceptions. Does it make sense to distinguish SNaNs and drop NaN propagation only for them?
> For ebStrict we leave the instruction alone and let the run-time environment handle it.
> 
> A check of IEEE 754 seems to say that, when given an SNaN, an operation that produces a result will produce a QNaN. But I'm not a 754 expert. Anyone else? Please?
> 
> If you look above at the propagateNaN() function you'll see that someone thought of this and didn't have an answer. They left a comment asking the question. Was that @spatel?
> 
> I don't think a conversion to QNaN should be confined to the constrained intrinsics. It would be surprising if, for example, someone changed to a different rounding mode and suddenly NaN handling changed. So I think we should keep the codepath here the same for constrained and regular floating point. And changing propagateNaN() should be left for a later patch. Does that sound reasonable?
Yes, I left that comment. That was partly discussed in D44521.
I still don't know what we want to do about SNaN. If we can make a stand-alone patch to answer just that question for the constrained intrinsics, that would be nice.


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

https://reviews.llvm.org/D103169