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

[andrewng]

Summary

One of the fast-math optimizations is to replace calls to standard double
functions with their float equivalents, e.g. exp -> expf. However, this can
cause infinite loops for the following:

float expf(float val) { return (float) exp((double) val); }

So this fix checks that the calling function to the standard double function
that is being replaced does not match the float equivalent.

[efriedma]

Can you explain a little more how exactly we end up in this situation? Your example has undefined behavior according to the C standard.

[andrewng]

In D31806#721631, @efriedma wrote:

Can you explain a little more how exactly we end up in this situation? Your example has undefined behavior according to the C standard.

The issue crops up on Mingw-w64 for x86_64 target, as their "math.h" header contains an inline definition of "expf" similar to my example. When compiling with "-O2 -ffast-math", functions calling "expf" are optimized to infinite loops. Is this the information you're looking for? Thanks.

[efriedma]

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?

lib/Transforms/Utils/SimplifyLibCalls.cpp

935 ↗

(On Diff #94510)

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.

945 ↗

(On Diff #94510)

The FunctionType check doesn't seem necessary here.

test/Transforms/Util/libcalls-fast-math-inf-loop.ll

1 ↗

(On Diff #94510)

Please don't use -O2 in tests... just test the specific pass you care about (in this case, instcombine).

[andrewng]

In 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.

lib/Transforms/Utils/SimplifyLibCalls.cpp

935 ↗

(On Diff #94510)

OK, will explicitly mention MinGW case.

945 ↗

(On Diff #94510)

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.

test/Transforms/Util/libcalls-fast-math-inf-loop.ll

1 ↗

(On Diff #94510)

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.

[efriedma]

On 32-bit Windows, the compiler is aware that the standard library doesn't have an expf implementation, and therefore doesn't treat it as a builtin function.

lib/Transforms/Utils/SimplifyLibCalls.cpp

945 ↗

(On Diff #94510)

In that case, we're not only calling the wrong expf, we're calling it with the wrong signature, so we still would want to suppress the transform.

test/Transforms/Util/libcalls-fast-math-inf-loop.ll

1 ↗

(On Diff #94510)

You can use opt -print-after-all to grab the IR before the bad transform.

[andrewng]

lib/Transforms/Utils/SimplifyLibCalls.cpp

945 ↗

(On Diff #94510)

Yes, of course! Sorry, completely forgot about name mangling and was thinking C++ source function names.

[andrewng]

I've updated the comments to explicitly mention the MinGW-w64 case that encounters this issue and removed the unnecessary FunctionType related checks.

The test has been updated to only include the passes that trigger the generation of the infinite loop. Is this OK, or should the test only focus on the instcombine pass not substituting the exp with expf?

[efriedma]

The test has been updated to only include the passes that trigger the generation of the infinite loop. Is this OK, or should the test only focus on the instcombine pass not substituting the exp with expf?

Ideally, just an instcombine test; the behavior of the inliner isn't really relevant here (and it's easy to get confused by the pass ordering in "-inline -instcombine -tailcallelim").

[andrewng]

Updated test to focus only on the instcombine pass.

[efriedma]

LGTM

llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp

if (CheckRetType) {
     }
   }
 
   // If this is something like 'floor((double)floatval)', convert to floorf.
   Value *V = valueHasFloatPrecision(CI->getArgOperand(0));
   if (V == nullptr)
     return nullptr;
   
+  // If call isn't an intrinsic, check that it isn't within a function with the
+  // same name as the float version of this call.
+  //
+  // e.g. inline float expf(float val) { return (float) exp((double) val); }
+  //
+  // A similar such definition exists in the MinGW-w64 math.h header file which
+  // when compiled with -O2 -ffast-math causes the generation of infinite loops
+  // where expf is called.
+  if (!Callee->isIntrinsic()) {
+    const Function *F = CI->getFunction();
+    StringRef FName = F->getName();
+    StringRef CalleeName = Callee->getName();
+    if ((FName.size() == (CalleeName.size() + 1)) &&
+        (FName.back() == 'f') &&
+        FName.startswith(CalleeName))
+      return nullptr;
+  }
+
   // Propagate fast-math flags from the existing call to the new call.
   IRBuilder<>::FastMathFlagGuard Guard(B);
   B.setFastMathFlags(CI->getFastMathFlags());
 
   // floor((double)floatval) -> (double)floorf(floatval)
   if (Callee->isIntrinsic()) {
     Module *M = CI->getModule();
     Intrinsic::ID IID = Callee->getIntrinsicID();

llvm/trunk/test/Transforms/Util/libcalls-fast-math-inf-loop.ll

+; RUN: opt -S -instcombine -o - %s | FileCheck %s
+
+; Test that fast math lib call simplification of double math function to float
+; equivalent doesn't occur when the calling function matches the float
+; equivalent math function. Otherwise this can cause the generation of infinite
+; loops when compiled with -O2/3 and fast math.
+
+; Test case C source:
+;
+;   extern double exp(double x);
+;   inline float expf(float x) { return (float) exp((double) x); }
+;   float fn(float f) { return expf(f); }
+;
+; IR generated with command:
+;
+;   clang -cc1 -O2 -ffast-math -emit-llvm -disable-llvm-passes -triple x86_64-unknown-unknown -o - <srcfile>
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-unknown"
+
+; Function Attrs: nounwind
+define float @fn(float %f) #0 {
+; CHECK: define float @fn(
+; CHECK: call fast float @expf(
+  %f.addr = alloca float, align 4
+  store float %f, float* %f.addr, align 4, !tbaa !1
+  %1 = load float, float* %f.addr, align 4, !tbaa !1
+  %call = call fast float @expf(float %1) #3
+  ret float %call
+}
+
+; Function Attrs: inlinehint nounwind readnone
+define available_externally float @expf(float %x) #1 {
+; CHECK: define available_externally float @expf(
+; CHECK: fpext float
+; CHECK: call fast double @exp(
+; CHECK: fptrunc double
+  %x.addr = alloca float, align 4
+  store float %x, float* %x.addr, align 4, !tbaa !1
+  %1 = load float, float* %x.addr, align 4, !tbaa !1
+  %conv = fpext float %1 to double
+  %call = call fast double @exp(double %conv) #3
+  %conv1 = fptrunc double %call to float
+  ret float %conv1
+}
+
+; Function Attrs: nounwind readnone
+declare double @exp(double) #2
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { inlinehint nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind readnone }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 5.0.0"}
+!1 = !{!2, !2, i64 0}
+!2 = !{!"float", !3, i64 0}
+!3 = !{!"omnipotent char", !4, i64 0}
+!4 = !{!"Simple C/C++ TBAA"}