This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by andrewng on Apr 7 2017, 5:03 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

andrewng created this revision.Apr 7 2017, 5:03 AM
efriedma edited edge metadata.Apr 7 2017, 3:07 PM

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

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.

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

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.

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 added inline comments.Apr 12 2017, 6:53 PM
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 updated this revision to Diff 95388.Apr 15 2017, 10:05 PM

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?

andrewng marked 3 inline comments as done.Apr 15 2017, 10:08 PM

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 this revision to Diff 95688.Apr 18 2017, 11:06 PM

Updated test to focus only on the instcombine pass.

This revision is now accepted and ready to land.Apr 19 2017, 11:47 AM
This revision was automatically updated to reflect the committed changes.