[PATCH] D70965: [SelectionDAG] Expand nnan FMINNUM/FMAXNUM to select sequence

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand marked 4 inline comments as done.
uweigand added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6234
+  // on libm into a file that originally did not have one.
+  if (Node->getFlags().hasNoNaNs()) {
+    ISD::CondCode Pred =
----------------
spatel wrote:
> I think this is an acceptable check of the FMF, but the reasoning is subtle. If we synthesized the intrinsic in IR, it required 'nsz' for the fcmp/select because the original code using fcmp can produce a different result for -0.0. 
> 
> And that is because the intrinsics have this clause:
> "If the operands compare equal, returns a value that compares equal to both operands. This means that fmin(+/-0.0, +/-0.0) could return either -0.0 or 0.0"
> 
> So once we have the intrinsic, the 'nsz' behavior becomes implicit. 
> 
> And since we're not checking for 'nsz' explicitly in this expansion, that means that we may be transforming code that originally had the libcall in source to inline code (the intrinsic was not created by InstCombine without 'nsz').
> 
> But that's probably a good optimization for a target that is expanding these nodes anyway?
> 
> To not lose an optimization opportunity, I think we need to add 'nsz' to the setFlags() call under here. Or avoid this complexity and check for 'nsz' in the first place.
I deliberately did not check nsz because I understood this to be implied anyway by the semantics of the FMINNUM/FMAXNUM nodes:

```

    /// The return value of (FMINNUM 0.0, -0.0) could be either 0.0 or -0.0.

```
I've now added code to explicitly set nsz on the select node.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6237-6239
+    SDValue Tmp1 = Node->getOperand(0);
+    SDValue Tmp2 = Node->getOperand(1);
+    Tmp1 = DAG.getSelectCC(dl, Tmp1, Tmp2, Tmp1, Tmp2, Pred);
----------------
spatel wrote:
> Variables named 'Tmp' make me nervous. :)
> I'd prefer to not reassign local names:
>   SDValue Op0 = Node->getOperand(0);
>   SDValue Op1 = Node->getOperand(1);
>   SDValue SelCC = DAG.getSelectCC(dl, Op0, Op1, Op0, Op1, Pred);
Sorry about that, too much copy-and-paste from LegalizeDAG :-)


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

https://reviews.llvm.org/D70965