[PATCH] D86801: [InstCombine] add extra-use tests for fmul+sqrt; NFC

[Venkataramanan Kumar via Phabricator via llvm-commits]

venkataramanan.kumar.llvm added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/fmul-sqrt.ll:121
+; CHECK-NEXT:    [[RSQRT:%.*]] = fdiv fast double 1.000000e+00, [[SQRT]]
+; CHECK-NEXT:    [[RES:%.*]] = fmul reassoc double [[RSQRT]], [[X:%.*]]
+; CHECK-NEXT:    store double [[RSQRT]], double* %p
----------------
spatel wrote:
> venkataramanan.kumar.llvm wrote:
> > venkataramanan.kumar.llvm wrote:
> > > spatel wrote:
> > > > This is not providing the test coverage that you intended - notice that the operands of fmul are swapped.
> > > > Please see my comment about complexity-based canonicalization in D86395.
> > > Hi Sanjay, 
> > > 
> > > The  function "SimplifyAssociativeOrCommutative" is called before we try to fold  x * 1/sqrt(x) to x/sqrt(x) in the function "visitFMul" .   
> > > 
> > > This function swaps the operand  x *1/sqrt(x)  to  1/sqrt(x) *x based on complexity values. 
> > > 
> > > Rule for commutative operator:  LHS (more complexity value )  Binary RHS (less complexity value).  
> > > 1/sqrt(x) has value 5 and x has value 2.
> > > 
> > > --Snip--
> > > bool InstCombinerImpl::SimplifyAssociativeOrCommutative(BinaryOperator &I) {
> > >   Instruction::BinaryOps Opcode = I.getOpcode();
> > >   bool Changed = false;
> > > 
> > >     // Order operands such that they are listed from right (least complex) to
> > >     // left (most complex).  This puts constants before unary operators before
> > >     // binary operators.
> > >     if (I.isCommutative() && getComplexity(I.getOperand(0)) <
> > >         getComplexity(I.getOperand(1)))
> > >       Changed = !I.swapOperands();
> > > ---Snip--
> > > 
> > > So looks like we don't need to match for x *1/sqrt(x)  in  [[https://reviews.llvm.org/D86726 |  D86726]].
> > > But for this test case patch , can we keep as an extra coverage test?
> > May be I should try a different test case  for  x * 1/sqrt(x) where x is an expression with same or higher complexity than 1/sqrt(x). 
> Right! Looks like you got it. Sorry if my earlier comment was not clear. I prefer to label that extra expression with a code comment (grep for "thwart" in this test directory), so other readers will have a better chance of understanding why the 'add' in this case is included in the test.
Oh!! I did not understand your earlier comment.  Instead of adding "thwart", I changed the test case.   

Also can you please commit on behalf of me. 


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

https://reviews.llvm.org/D86801