[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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86801