[PATCH] D87877: [InstCombine] Fix errno bug in pow expansion to sqrt

Hubert Tong via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 21 10:20:14 PDT 2020


hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1735-1737
   // pow(x, n) -> x * x * x * ...
   const APFloat *ExpoF;
+  if (!IsSqrtOrSqrtReciprocal && AllowApprox && match(Expo, m_APFloat(ExpoF))) {
----------------
spatel wrote:
> This transform is making this patch more complicated, right?
> How about adding an explicit check to avoid it for the single sqrt case (that could be a preliminary NFC cleanup patch if you prefer). Then we just add the more obvious correctness check inside of replacePowWithSqrt():
> 
> ```
> diff --git a/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp b/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
> index 60b7da7e64f..4ce580f47ad 100644
> --- a/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
> +++ b/llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
> @@ -1636,6 +1636,14 @@ Value *LibCallSimplifier::replacePowWithSqrt(CallInst *Pow, IRBuilderBase &B) {
>    if (ExpoF->isNegative() && (!Pow->hasApproxFunc() && !Pow->hasAllowReassoc()))
>      return nullptr;
>  
> +  // If we have a pow() library call (accesses memory) and we can't guarantee
> +  // that the base is not Inf, give up:
> +  // pow(-Inf, 0.5) does not set errno (result is +Inf as shown below), but
> +  // sqrt(-Inf) may set errno.
> +  if (!Pow->doesNotAccessMemory() && !Pow->hasNoInfs() &&
> +      !isKnownNeverInfinity(Base, TLI))
> +    return nullptr;
> +
>    Sqrt = getSqrtCall(Base, Attrs, Pow->doesNotAccessMemory(), Mod, B, TLI);
>    if (!Sqrt)
>      return nullptr;
> @@ -1722,7 +1730,8 @@ Value *LibCallSimplifier::optimizePow(CallInst *Pow, IRBuilderBase &B) {
>  
>    // pow(x, n) -> x * x * x * ...
>    const APFloat *ExpoF;
> -  if (AllowApprox && match(Expo, m_APFloat(ExpoF))) {
> +  if (AllowApprox && match(Expo, m_APFloat(ExpoF)) &&
> +      !ExpoF->isExactlyValue(0.5) && !ExpoF->isExactlyValue(-0.5)) {
>      // We limit to a max of 7 multiplications, thus the maximum exponent is 32.
>      // If the exponent is an integer+0.5 we generate a call to sqrt and an
>      // additional fmul.
> 
> ```
I'll look into committing the check as an NFC cleanup. I'll probably also move this transform into its own function (like the earlier transforms) as an NFC cleanup too. That would make the (possibly unintended) different handling of the early exit cases from this transformation (compared to the other ones) more obvious.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87877/new/

https://reviews.llvm.org/D87877



More information about the llvm-commits mailing list