[PATCH] D31806: [SimplifyLibCalls] Fix infinite loop with fast-math optimization.

[Andrew Ng via Phabricator via llvm-commits]

andrewng added a comment.

In https://reviews.llvm.org/D31806#722732, @efriedma wrote:

> gcc transforms the given C code to a call to expf(), just like LLVM, but then it looks like their inliner works a bit differently so the generated code ends up with a call to expf() rather than an infinite loop.  With your patch, LLVM also generates a call to expf().  This seems weird... does MinGW's C library actually have an expf() function?


I believe that the 64-bit C library does have an expf(), but the 32-bit may not (or perhaps didn't in the past). So yes, in some ways this could be considered an issue with MinGW's "math.h" header. At the same time, preventing the generation of infinite loops seems reasonable too. I'm away on holiday (vacation) right now, so may not be able to update until I get back on 25th April. Thanks.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:935
+  // same name and signature as the float version of this call!
+  // e.g. float floorf(float val) { return (float) floor((double) val); }
+  if (!Callee->isIntrinsic()) {
----------------
efriedma wrote:
> You might want to explicitly note that MinGW is doing this; otherwise it seems like a weird hack, because it makes no sense to write an inline function like this if you have a standard-compliant C library.
OK, will explicitly mention MinGW case.


================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:945
+      if (FT->getReturnType()->isFloatTy() && (FT->getNumParams() == 1) &&
+          FT->getParamType(0)->isFloatTy())
+        return nullptr;
----------------
efriedma wrote:
> The FunctionType check doesn't seem necessary here.
Wouldn't this be more correct? There's always the remote possibility that someone might have a function named expf with a different signature that calls exp.


================
Comment at: test/Transforms/Util/libcalls-fast-math-inf-loop.ll:1
+; RUN: opt -S -O2 -o - %s | FileCheck %s
+
----------------
efriedma wrote:
> Please don't use -O2 in tests... just test the specific pass you care about (in this case, instcombine).
Yes, I wasn't too happy with this either. However, just the instcombine wasn't enough to cause the infinite loop. I will try to figure out what else is needed to trigger this behaviour.


https://reviews.llvm.org/D31806