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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1644
+  if (!Pow->doesNotAccessMemory() && !Pow->hasNoInfs() &&
+      !isKnownNeverInfinity(Base, TLI))
+    return nullptr;
----------------
@spatel, I've added the `isKnownNeverInfinity` check here as suggested in your comment below. I am not entirely sure whether it is quite effective though. It seems this query may happening "too early".

Example case I tried (with `opt -instcombine`):
```
define double @pow_libcall_half_no_FMF_base_ninf(i32 %x) {
  %conv = sitofp i32 %x to double
  %pow = call double @pow(double %conv, double 5.0e-01)
  ret double %pow
}
```


================
Comment at: llvm/test/Transforms/InstCombine/pow-sqrt.ll:31
 
-; This makes no difference, but FMF are propagated.
+; This makes no difference, but FMF are propagated/retained.
 
----------------
hubert.reinterpretcast wrote:
> spatel wrote:
> > This comment should be more like above:
> >   ; The transform to sqrt is not allowed if we risk setting errno due to -INF.
> > 
> > Although that raises a question: if the user has allowed an approximation of pow(), do they really expect that errno would be set accurately? Similarly, if they allowed 'reassoc'...
> Given NaN propagation, if the `sqrt` was okay, then the result here should be NaN. That is, I think the question is more "can we just do the transform and omit the select"?
I've updated the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87877