[PATCH] D40150: [LibCallSimplifier] fix pow(x, 0.5) -> sqrt() transforms

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D40150#928146, @hfinkel wrote:

> In https://reviews.llvm.org/D40150#928114, @efriedma wrote:
>
> > > 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?


https://reviews.llvm.org/D40150