This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Use less bitwise operations to handle Instruction::SExt in SimplifyDemandedUseBits. Other improvements.
ClosedPublic

Authored by craig.topper on Apr 14 2017, 3:10 PM.

Details

Summary

The current code created a NewBits mask and used it as a mask several times. One of them just before a call to trunc making it unnecessary. A call to getActiveBits can get us the same information for the case. We also ORed with this mask later when we should have just sign extended the known bits.

We also called trunc on the guaranteed to be zero KnownZeros/Ones masks entering this code. Creating appropriately sized temporary APInts is probably better. (This needs to be done in the zero extend code too which I'll do as a follow up)

We also made a recursive call to SimplifyDemandedBits even if we were just going to replace it with ZExt due to upper bits not being used. Unfortunately, now we create the ZExt in two separate places and I didn't see a great way to merge the control flow.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 14 2017, 3:10 PM
majnemer added inline comments.Apr 14 2017, 3:36 PM
lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
437 ↗(On Diff #95350)

Using isNegative on InputKnownZero is a little confusing because we are testing to see if the sign bit is clear.

craig.topper added inline comments.Apr 14 2017, 3:45 PM
lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
437 ↗(On Diff #95350)

Yeah, I'd really like to say "getSignBit()" Maybe I should at that as an alias in APInt. Assuming its not confusing with the separate APInt::getSignBit(unsigned BitWidth) that creates a new APInt with just the sign bit set. Which is kind of terribly named in itself. Should be getSignBitMask.

Use APInt::isSignBitSet instead of isNegative

davide requested changes to this revision.Apr 25 2017, 2:06 PM

To be honest, I'm not entirely sure this cleanup is a net win. The fact we have to duplicate the zext logic is a little annoying.
I don't feel strongly about rejecting this patch, but I don't feel strongly about accepting either. I guess David as instcombine owner has the final word on this one.

This revision now requires changes to proceed.Apr 25 2017, 2:06 PM

I can remove the Zext duplication. Would that make it better?

I can remove the Zext duplication. Would that make it better?

Probably, IMHO.

craig.topper edited edge metadata.

Remove the early conversion to ZExtInst.

spatel accepted this revision.May 24 2017, 8:13 AM

LGTM.

Not sure what phab protocol requires: @majnemer 's feedback is addressed by the new API (isNonNegative), but you must wait for @davide to respond since he requested changes?

davide accepted this revision.May 24 2017, 9:55 AM
This revision is now accepted and ready to land.May 24 2017, 9:55 AM
This revision was automatically updated to reflect the committed changes.