This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine/InstSimplify] Move ashr optimization from Instcombine to Instsimplify
ClosedPublic

Authored by suyog on Jun 11 2014, 6:20 AM.

Details

Summary

This patch moves the following optimization from visitAShr to SimplifyAShrInst :

// Arithmetic shifting an all-sign-bit value is a no-op.
unsigned NumSignBits = ComputeNumSignBits(Op0);
if (NumSignBits == Op0->getType()->getScalarSizeInBits())
  return Op0;

No regression observed. No functionality change.
This patch is part of refactoring code which i have planned along with Rahul Jain.

Primarily, we are targeting code which returns simply value instead of new instruction.

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

Please help review this patch.

Thanks,
Suyog

Diff Detail

Repository
rL LLVM

Event Timeline

suyog updated this revision to Diff 10321.Jun 11 2014, 6:20 AM
suyog retitled this revision from to [InstCombine/InstSimplify] Move ashr optimization from Instcombine to Instsimplify.
suyog updated this object.
suyog edited the test plan for this revision. (Show Details)
suyog added reviewers: rafael, majnemer, bkramer.
suyog added a subscriber: Unknown Object (MLST).
suyog added a comment.Jun 17 2014, 5:34 AM

Gentle Ping !!

suyog updated this revision to Diff 10560.Jun 18 2014, 6:19 AM
suyog added a reviewer: nlewycky.

Hi Nick,

Added supporting testcase.

Please if you could commit it for me if it looks
good to you.

Thanks,
Suyog

suyog updated this revision to Diff 10926.EditedJun 27 2014, 5:31 AM

Hi Nick,

I modified the test case to use FileCheck.

I also believe the test case is in accordance to what we taught instsimplify as mentioned by Rahul above.
Also, this test case was the original one for instcombine, the code for which we moved to instsimplify.

Can you point out what's specific wrong in the test case? Your inputs are most welcomed :)

suyog updated this revision to Diff 10970.Jun 29 2014, 9:04 PM

Small correction.

The test case is for 'instsimplify' rather than 'instcombine'.

Corrected the patch.

suyog added a comment.Jul 7 2014, 11:39 AM

Gentle Ping !!

suyog added a comment.Jul 14 2014, 3:23 AM

Gentle Ping !!

Nick, can you please help in reviewing it? Just a small patch :)

nicholas accepted this revision.Jul 16 2014, 1:15 AM
nicholas added a reviewer: nicholas.
nicholas added a subscriber: nicholas.

Merge the test into something else (binops.ll perhaps)?

This revision is now accepted and ready to land.Jul 16 2014, 1:15 AM
nlewycky edited edge metadata.Jul 16 2014, 1:18 AM

And LGTM. (I forgot that selecting "accept this revision" in the web UI
does not add LGTM to the email message.)

suyog closed this revision.Jul 16 2014, 11:37 PM
suyog updated this revision to Diff 11559.

Closed by commit rL213231 (authored by @suyog).

llvm/trunk/lib/Transforms/InstCombine/InstCombineShifts.cpp