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

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



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


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

https://reviews.llvm.org/D86801