[PATCH] D86726: [InstCombine]: Transform 1.0/sqrt(X) * X to X/sqrt(X) and X * 1.0/sqrt(X) to X/sqrt(X)

Tue Sep 1 08:11:20 PDT 2020

venkataramanan.kumar.llvm added inline comments.

================
Comment at: llvm/test/Transforms/InstCombine/fmul-sqrt.ll:110-112
   %sqrt = call fast double @llvm.sqrt.f64(double %x)
   %rsqrt = fdiv fast double 1.0, %sqrt
   %res = fmul reassoc double %rsqrt, %x
----------------
spatel wrote:
> venkataramanan.kumar.llvm wrote:
> > spatel wrote:
> > > This test could provide better coverage for the logic as implemented. The only FMF requirement is the "reassoc" (and likely "nsz") on the fmul, and that's typical of our FP folds so far. 
> > > 
> > > If we want to make that more conservative by requiring FMF on the fdiv or fsqrt too, this test would then offer a potential counter-example.
> > > 
> > > There's some discussion about the semantics/subtlety of FMF in:
> > > https://llvm.org/PR46326
> > is there any restriction that FMF should be set on all the operands we try to re-associate ?  
> > 
> > When I read the PR it seems there is no such rule.  
> > 
> > For the patch I will add "nsz" to fmul.  It already has the "fast" setting for other operations.  is that the correct understanding?
> > 
> No, there is no rule requiring FMF on all operands in the expression. The rules are currently unspecified as to exactly how FMF should be predicated/applied. That's what makes this and related transforms potentially controversial and subject to change in the future.
> 
> I think it is easier to show my suggestion directly, so I updated the tests here:
> rGd48699e
> 
> The 1st test now has the minimum FMF required to trigger the transform; the 2nd test is what we would typically expect -ffast-math code to look like (everything is full 'fast').
> 
> (Note: I also changed the 2nd test to use vector types for better test coverage.)
> 
> Please have a look and rebase. Let me know if it makes sense.
yes that makes sense.  Updated the patch after rebase.

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

https://reviews.llvm.org/D86726