This is an archive of the discontinued LLVM Phabricator instance.

[LibCallSimplifier] fix pow(x, 0.5) -> sqrt() transforms
ClosedPublic

Authored by spatel on Nov 16 2017, 3:30 PM.

Details

Summary

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

As the first test shows, we could transform an llvm intrinsic into a libcall which could then set errno, so that's just wrong...unless I'm not seeing some loophole. The second test diff is a phantom change (same output as before), but I've updated the CHECK lines to highlight that we get that case right on purpose now.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Nov 16 2017, 3:30 PM
efriedma edited edge metadata.Nov 16 2017, 4:25 PM

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

lib/Transforms/Utils/SimplifyLibCalls.cpp
1080 ↗(On Diff #123248)

Probably something like ninf nsz? But let's do that in a followup.

hfinkel edited edge metadata.Nov 16 2017, 4:33 PM

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

We should, at some point soon, come up with a real plan to handle this problem. On systems that don't have appropriate functions, we might generate wrappers that save/restore errno and call those wrappers instead of the libm functions directly. For -gnu systems, we can/should generate calls to pow_finite instead of pow and similar for other functions (these are in /usr/include/bits/math-finite.h and are what you generally get from math.h when FINITE_MATH_ONLY__ is positive).

spatel added inline comments.Nov 17 2017, 6:47 AM
lib/Transforms/Utils/SimplifyLibCalls.cpp
1080 ↗(On Diff #123248)

That might be it. As you can tell from the other tests that I added, I initially set out to make this transform a potential test of the meaning of the new 'afn' FMF...but then I got stuck in another batch of errno quicksand. :)

This would be a lot simpler if we could disregard errno when we have relaxed FP math, but I'm assuming it's possible to do something like '-ffast-math -fmath-errno' and still expect errno to be set for domain errors?

nnan implies we don't care about the result in cases where it would be nan, so it's probably okay if we lower an nnan "pow(x, 0.5)" to something which never sets errno. But there's still the underlying problem that the backend can't lower the intrinsics correctly for targets where libm sets errno, so we shouldn't generate libm intrinsics for code which isn't already using them.

We should, at some point soon, come up with a real plan to handle this problem. On systems that don't have appropriate functions, we might generate wrappers that save/restore errno and call those wrappers instead of the libm functions directly. For -gnu systems, we can/should generate calls to pow_finite instead of pow and similar for other functions (these are in /usr/include/bits/math-finite.h and are what you generally get from math.h when FINITE_MATH_ONLY__ is positive).

Let me check if I have this right for this example: We can't safely make progress here currently - we must at least add a TLI hook like "bool canLowerCallWithoutSettingErrno(CallInst *)". Without that, we have this potential:

  1. The incoming call is llvm.pow() - can't set errno.
  2. We transform it to llvm.sqrt() here in IR.
  3. The backend lowers it to a libcall sqrt() which can set errno.

Given that, should we:

  1. Go ahead with this patch as-is? We're at least avoiding the case where we knowingly turn intrinsic llvm.pow() into libcall sqrt() in IR.
  2. Simplify this patch to transform both pow() and llvm.pow() into llvm.sqrt() given the protective cover of isFast()?
  3. Postpone this patch until we have the TLI hook and backend support to do the right thing?
efriedma accepted this revision.Nov 17 2017, 12:28 PM

I think this is okay for now; we can clean it up when the backend is fixed.

This revision is now accepted and ready to land.Nov 17 2017, 12:28 PM
This revision was automatically updated to reflect the committed changes.