Page MenuHomePhabricator

[InstCombine] add extra-use tests for fmul+sqrt; NFC
ClosedPublic

Authored by venkataramanan.kumar.llvm on Aug 28 2020, 12:04 PM.

Details

Reviewers
spatel
Summary

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

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2020, 12:04 PM
venkataramanan.kumar.llvm requested review of this revision.Aug 28 2020, 12:04 PM
spatel added inline comments.Aug 28 2020, 1:13 PM
llvm/test/Transforms/InstCombine/fmul-sqrt.ll
121

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.

llvm/test/Transforms/InstCombine/fmul-sqrt.ll
121

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 D86726.
But for this test case patch , can we keep as an extra coverage test?

llvm/test/Transforms/InstCombine/fmul-sqrt.ll
121

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

As per the comment given by Sanjay, updated the coverage test for x*1/sqrt(x) pattern.

spatel accepted this revision.Aug 29 2020, 9:37 AM

LGTM

llvm/test/Transforms/InstCombine/fmul-sqrt.ll
121

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.

This revision is now accepted and ready to land.Aug 29 2020, 9:37 AM
llvm/test/Transforms/InstCombine/fmul-sqrt.ll
121

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.

This comment was removed by venkataramanan.kumar.llvm.

Added comment in the test case.

spatel closed this revision.Aug 30 2020, 3:43 PM

Committed here:
rG86163f885b5

Please request commit access so you're not waiting on others to push an approved patch.