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

[craig.topper]

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.

[majnemer]

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

410

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

[craig.topper]

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

410

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.

[craig.topper]

Use APInt::isSignBitSet instead of isNegative

[davide]

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.

[craig.topper]

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

[davide]

In D32098#737344, @craig.topper wrote:

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

Probably, IMHO.

[craig.topper]

Remove the early conversion to ZExtInst.

[craig.topper]

Rebase

[craig.topper]

Ping

[spatel]

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?

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

case Instruction::ZExt: {
     KnownOne = KnownOne.zext(BitWidth);
     assert(!(KnownZero & KnownOne) && "Bits known to be one AND zero?");
     // The top bits are known to be zero.
     KnownZero.setBitsFrom(SrcBitWidth);
     break;
   }
   case Instruction::SExt: {
     // Compute the bits in the result that are not present in the input.
-    unsigned SrcBitWidth =I->getOperand(0)->getType()->getScalarSizeInBits();
+    unsigned SrcBitWidth = I->getOperand(0)->getType()->getScalarSizeInBits();
 
-    APInt InputDemandedBits = DemandedMask &
-                              APInt::getLowBitsSet(BitWidth, SrcBitWidth);
+    APInt InputDemandedBits = DemandedMask.trunc(SrcBitWidth);
 
-    APInt NewBits(APInt::getBitsSetFrom(BitWidth, SrcBitWidth));
     // If any of the sign extended bits are demanded, we know that the sign
     // bit is demanded.
-    if ((NewBits & DemandedMask) != 0)
-      InputDemandedBits.setBit(SrcBitWidth-1);
+    if (DemandedMask.getActiveBits() > SrcBitWidth)
+      InputDemandedBits.setSignBit();
 
-    InputDemandedBits = InputDemandedBits.trunc(SrcBitWidth);
-    KnownZero = KnownZero.trunc(SrcBitWidth);
-    KnownOne = KnownOne.trunc(SrcBitWidth);
-    if (SimplifyDemandedBits(I, 0, InputDemandedBits, KnownZero, KnownOne,
-                             Depth + 1))
+    APInt InputKnownZero(SrcBitWidth, 0), InputKnownOne(SrcBitWidth, 0);
+    if (SimplifyDemandedBits(I, 0, InputDemandedBits, InputKnownZero,
+                             InputKnownOne, Depth + 1))
       return I;
-    InputDemandedBits = InputDemandedBits.zext(BitWidth);
-    KnownZero = KnownZero.zext(BitWidth);
-    KnownOne = KnownOne.zext(BitWidth);
-    assert(!(KnownZero & KnownOne) && "Bits known to be one AND zero?");
-
-    // If the sign bit of the input is known set or clear, then we know the
-    // top bits of the result.
 
     // If the input sign bit is known zero, or if the NewBits are not demanded
     // convert this into a zero extension.
-    if (KnownZero[SrcBitWidth-1] || (NewBits & ~DemandedMask) == NewBits) {
-      // Convert to ZExt cast
+    if (InputKnownZero.isSignBitSet() ||
+        DemandedMask.getActiveBits() <= SrcBitWidth) {
+      // Convert to ZExt cast.
       CastInst *NewCast = new ZExtInst(I->getOperand(0), VTy, I->getName());
       return InsertNewInstWith(NewCast, *I);
-    } else if (KnownOne[SrcBitWidth-1]) {    // Input sign bit known set
-      KnownOne |= NewBits;
     }
+
+    // If the sign bit of the input is known set or clear, then we know the
+    // top bits of the result.
+    KnownZero = InputKnownZero.sext(BitWidth);
+    KnownOne = InputKnownOne.sext(BitWidth);
+    assert(!(KnownZero & KnownOne) && "Bits known to be one AND zero?");
     break;
   }
   case Instruction::Add:
   case Instruction::Sub: {
     /// If the high-bits of an ADD/SUB are not demanded, then we do not care
     /// about the high bits of the operands.
     unsigned NLZ = DemandedMask.countLeadingZeros();
     if (NLZ > 0) {