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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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 =
----------------
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.


================
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);
----------------
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);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70965