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

[Sanjay Patel via Phabricator via llvm-commits]

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

```


================
Comment at: llvm/test/Transforms/InstCombine/pow-1.ll:269
 ;
-  %retval = call float @powf(float %x, float 0.5)
+  %retval = call ninf float @powf(float %x, float 0.5)
   ret float %retval
----------------
I realize this will be a little more work, but it would be better to replicate this test with the additional FMF (and similarly for other tests that are changing the flags and/or libcall/intrinsic).

That way, we'll retain the likely original intent of the test and add coverage for the cases that we want to verify are not miscompiled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87877