[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/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
----------------
hubert.reinterpretcast wrote:
> spatel wrote:
> > 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.
> I'm not convinced the coverage is meaningfully being lost. There is already a fairly exhaustive combination of tests in `pow-sqrt.ll`. The `double` version of this test (with no FMF) appears twice already.
> 
> I meant what I said when I wrote that the changes are made "depending on emphasis in the checks for library calls, avoidance of overlap, and overall coverage". Following that guideline, I found the specific test changes to be rather deterministic.
OK. I agree that there are a lot of tests for this transform (because it's been shown buggy even before the bug you found). If we can organize it better that would be great, but that doesn't need to hold up the fix for a miscompile.


================
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.
 
----------------
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'...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87877