[PATCH] Combine fcmp + select to fminnum / fmaxnum if no nans and legal

Ahmed Bougacha ahmed.bougacha at gmail.com
Mon Jan 12 15:58:17 PST 2015


+Owen

In http://reviews.llvm.org/D6744#107756, @arsenm wrote:

> In http://reviews.llvm.org/D6744#107740, @ab wrote:
>
> > The NaN part LGTM, but what happens on 0?
> >
> > Say:
> >
> >   select +0.0, -0.0, (fcmp lt +0.0, -0.0)
> >
> > turns into:
> >
> >   fminnum +0.0, -0.0
> >   
> >
> > My understanding is, the first returns -0.0 (because they compare equal, and not "lt").
> >  But it's unspecified which the second returns, so if the implementation returns the first operand when both are zero, it would return a "different" result here, +0.0.
> >  Does that make sense?
>
>
> Yes, however I am unclear on what guarantees there are for signed zeros. The DAG TargetOptions are also missing the equivalent of No Signed Zeros. I can weaken this to unsafe FP Math as well.


I believe the X86 equivalent of this combine swaps operands to make sure it gets the same result, but that's only possible because the operation is specified there.

Other knowledgeable FP people, an opinion?

-Ahmed


REPOSITORY
  rL LLVM

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4743
@@ +4742,3 @@
+    const TargetOptions &Options = DAG.getTarget().Options;
+    if (Options.NoNaNsFPMath && VT.isFloatingPoint() && N0.hasOneUse()) {
+      ISD::CondCode CC = cast<CondCodeSDNode>(N0.getOperand(2))->get();
----------------
arsenm wrote:
> ab wrote:
> > Optionally, how about checking isKnownNeverNaN for both operands?
> > Also, the non-U/-O CondCodes don't care about NaNs, so it would be fine to bypass NoNaNsFPMath in that case as well, no?  Though I'm not sure that combination ever happens anyway.
> I didn't know about isKnownNeverNaN, I'll switch to using it.
> 
> They do care about NaN. The ordered compares fail if either operand is a NaN, and the unordered succeed if either is.
I meant the non-orderered, and non-unordered, compares, as e.g., ISD::SETLT, ISD::SETGT. Those are undefined on NaNs, no?

http://reviews.llvm.org/D6744

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list