This is an archive of the discontinued LLVM Phabricator instance.

[Instcombine/InstSimplify] Move optimization from instcombine to instsimplify
ClosedPublic

Authored by rahul.jain on Jun 11 2014, 3:33 AM.

Details

Summary

Hi all,

This patch moves the following optimization from visitOr to SimplifyOrInst.
(A & C1)|(B & C2)
If we have: ((V + N) & C1) | (V & C2)
.. and C2 = ~C1 and C2 is 0+1+ and (N & C2) == 0.

This follows the statement that: InstComb optimization which do not create new instructions should
be moved to InstructionSimplify.

Suyog and myself have planned to come up with more such refactoring patches as and when we
encounter possible cases in InstComb which should be moved to InstSimplify.

Any suggestions/help/pointers on the same will be very much appreciated.

Please help review this patch.

Thanks,
Rahul

Diff Detail

Event Timeline

rahul.jain updated this revision to Diff 10319.Jun 11 2014, 3:33 AM
rahul.jain retitled this revision from to [Instcombine/InstSimplify] Move optimization from instcombine to instsimplify .
rahul.jain updated this object.
rahul.jain edited the test plan for this revision. (Show Details)
rahul.jain added a subscriber: Unknown Object (MLST).

gentle ping.

Inputs on this would be helpful. Is this heading in the
right direction?

Thanks,
Rahul

This needs a test to show what it does. There is a change in behaviour to be tested, opt -instsimplify will start doing something it didn't use to and opt -instcombine should stay the same.

lib/Analysis/InstructionSimplify.cpp
1630

Please sink this down to where they're first assigned (lines 1634, 1635).

1640

Fold this into the if-statement on line 1636 to reduce indentation one level.

rahul.jain updated this revision to Diff 10565.Jun 18 2014, 7:18 AM
rahul.jain updated this object.

Hi Nick,

Thanks a lot for the review.
Updated patch as per your comment.

  1. Sinked variable declarations next to their usage.
  2. Folded if statements.
  3. Added testcase.

Please help review the same.
If it looks good to you, please commit it for me.
Soon will ask for commit access.

Thanks,
Rahul

I made a few adjustments:

+ ConstantInt *C1 = nullptr, *C2 = nullptr;
+ C1 = dyn_cast<ConstantInt>(C);
+ C2 = dyn_cast<ConstantInt>(D);
became
+ ConstantInt *C1 = dyn_cast<ConstantInt>(C);
+ ConstantInt *C2 = dyn_cast<ConstantInt>(D);

I removed the dead initialization of V1 and V2 to nullptr. They aren't used
unless they match succeeds.

Your test had @test1 and @test3, renamed @test3 to @test2.

Changed test to use FileCheck instead of grep. I'm aware this is what
InstCombine/apint-or{1,2}.ll does, but it's an older style and FileCheck is
preferred.

Moved the whole-file comments that apply to each function ("the first
test..." and "the second test...") next to the functions they apply to.

Committed in r211252. Thanks!

Nick

rahul.jain accepted this revision.Jun 18 2014, 9:44 PM
rahul.jain added a reviewer: rahul.jain.
This revision is now accepted and ready to land.Jun 18 2014, 9:44 PM
rahul.jain closed this revision.Jun 18 2014, 9:45 PM

Committed as r211252 by Nick.

Thanks!