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

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

kpn 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));
----------------
spatel wrote:
> 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.
You want the regular FP instructions to be different from the constrained intrinsics? What about the rounding mode example above? Perhaps have NaN be handled differently for maytrap or strict exception handling? Still, I'm pretty sure we should keep the regular and constrained FP operations behaving the same whenever possible.

I think I can have a new patch up for discussion not too long after this patch lands.


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

https://reviews.llvm.org/D103169