Page MenuHomePhabricator

[InstCombine] canonicalize MUL with NEG operand
ClosedPublic

Authored by shchenz on Dec 20 2018, 3:15 PM.

Details

Summary

There is an opportunity in instCombine for following instruction pattern:

%mul = mul nsw i32 %b, %a
%cmp = icmp sgt i32 %mul, -1
%sub = sub i32 0, %a
%mul2 = mul nsw i32 %sub, %b
%cond = select i1 %cmp, i32 %mul, i32 %mul2

Source code for above pattern:

return (a*b) >=0 ? (a*b) : -a*b;

Currently, llvm(-O3) can not recognize this as abs(a*b).

Got help from @spatel in llvm-dev:
"
it looks like we are missing an IR canonicalization for 'mul' like this:
https://rise4fun.com/Alive/ARs

%neg = sub i8 0, %x
%r = mul i8 %neg, %y

-->

%mul2 = mul i8 %x, %y
%r = sub i8 0, %mul2

"

Testcases are already add in https://reviews.llvm.org/rL349847.
Thanks @jsji, i have updated the testcases according to his comment to the above nfc patch.

Diff Detail

Repository
rL LLVM

Event Timeline

shchenz created this revision.Dec 20 2018, 3:15 PM

Thank you for making the patch.

  1. Please always upload your patch with full context:

https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

  1. I don't think propagation of 'nsw' is safe in this transform:

https://rise4fun.com/Alive/wQc

  1. We need to check "m_OneUse" on the negated operand, or this transform will create an extra instruction. We should have 2 more tests with that pattern (and they should show that the transform does not occur). You might want to look at the code and tests for the similar transforms that we do for "fmul" later in this file.
shchenz updated this revision to Diff 179632.Dec 28 2018, 6:10 AM

fix according to Sanjay's comments

@spatel Hi Sanjay, thanks very much for your comments. I have updated accordingly. Could you please help to have another review? Thanks again.

Looks better.

llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
247–253 ↗(On Diff #179632)

This seems correct now, but i wonder if it would be better to fold this into a single branch:

// -X * Y --> -(X * Y)
if (match(&I, m_c_Mul(m_OneUse(m_Neg(m_Value(X))), m_Value(Y))))
  return BinaryOperator::CreateNeg(Builder.CreateMul(X, Y));
llvm/test/Transforms/InstCombine/mul.ll
457–468 ↗(On Diff #179632)

I almost complained that this is a regression.
Can you please split the NFC test update into a separate preparatory differential,
and only *regenerate* the check lines here?

shchenz updated this revision to Diff 179685.Dec 29 2018, 4:43 AM

fix according to Roman's comments.

Hi Roman @lebedev.ri, I have updated according to your comments. Updated testcases are committed in https://reviews.llvm.org/rL350154. Thanks very much.

lebedev.ri added inline comments.Dec 29 2018, 9:07 AM
llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
247 ↗(On Diff #179685)

That || looks confusing. Maybe just keep that as two lines?

llvm/test/Transforms/InstCombine/operand-complexity.ll
4 ↗(On Diff #179685)

I guess this test no longer serves it's purpose.
Not sure what to do here.

shchenz updated this revision to Diff 179717.Dec 29 2018, 3:55 PM

fix Roman's comments

shchenz marked an inline comment as done.Dec 29 2018, 3:59 PM

@lebedev.ri Updated. Thanks.

llvm/test/Transforms/InstCombine/operand-complexity.ll
4 ↗(On Diff #179685)

I changed the binop here to keep original testcases. It will not change these cases testing point.
new testcases for MUL canonicalization are all in file mul.ll.

lebedev.ri accepted this revision.Dec 29 2018, 10:14 PM

LGTM, thanks.
I think this is good to go.
Not sure whether you want to wait for another review.

This revision is now accepted and ready to land.Dec 29 2018, 10:14 PM
spatel accepted this revision.Dec 31 2018, 6:16 AM

LGTM

This revision was automatically updated to reflect the committed changes.