[PATCH] D11866: transform fmin/fmax calls when possible (PR24314)

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1221
@@ +1220,3 @@
+    if (Attr.getValueAsString() != "true")
+      return nullptr;
+    // No-signed-zeros is implied by the definitions of fmax/fmin themselves.
----------------
Actually, why do we need no NaNs? We don't support FP exceptions, so we only need to do the correct thing with NaN arguments (by returning the non-NaN). This should be easy to guarantee by picking the right ordered vs. unordered fcmp predicate.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1223
@@ +1222,3 @@
+    // No-signed-zeros is implied by the definitions of fmax/fmin themselves.
+    FMF.setNoSignedZeros();
+    FMF.setNoNaNs();
----------------
What in the definition implies this?

================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1233
@@ +1232,3 @@
+  Value *Op1 = CI->getArgOperand(1);
+  Value *Cmp = Callee->getName().startswith("fmin") ?
+    B.CreateFCmpOLT(Op0, Op1) : B.CreateFCmpOGT(Op0, Op1);
----------------
spatel wrote:
> hfinkel wrote:
> > You still need to check TLI->has(LibFunc::*) for the function itself. Maybe you want to make a hasBinaryFloatFn (like the existing hasUnaryFloatFn)?
> > 
> Sorry, I'm not understanding. We're replacing the call with regular instructions if we get this far, so there's no new library function to check. We check that the original code has this LibFunc before entering optimizeFMinMax(). Do we want to assert that or is there something else to check?
Ah, you're right: That is already checked in optimizeCall. I retract my comment ;)


http://reviews.llvm.org/D11866