[llvm-commits] SHR Patch (Updated)
Chris Lattner
clattner at apple.com
Sun Nov 5 11:36:14 PST 2006
On Nov 3, 2006, at 4:52 PM, Zhou Sheng wrote:
> Hi, Chris,
>
> i'm Sheng. Here are the SHR patches modified according to your
> suggestion. Please review them, thanks:)
>
> ***NOTE: This is for review only, please don't commit any of this.
>
Hi Sheng,
This is looking really good. In the InstCombine.cpp file, there are
still places where you can eliminate more casts and there is still a
bug. Notes below,
-Chris
@@ -1117,21 +1127,43 @@ bool InstCombiner::SimplifyDemandedBits(
...
+ case Instruction::AShr:
// If this is an arithmetic shift right and only the low-bit is
set, we can
// always convert this into a logical shr, even if the shift
amount is
// variable. The low bit of the shift cannot be an input sign
bit unless
// the shift amount is >= the size of the datatype, which is
undefined.
- if (DemandedMask == 1 && I->getType()->isSigned()) {
+ if (DemandedMask == 1) {
// Convert the input to unsigned.
Value *NewVal = InsertCastBefore(I->getOperand(0),
I->getType()-
>getUnsignedVersion(), *I);
// Perform the unsigned shift right.
- NewVal = new ShiftInst(Instruction::Shr, NewVal, I->getOperand
(1),
+ NewVal = new ShiftInst(Instruction::LShr, NewVal, I->getOperand
(1),
I->getName());
InsertNewInstBefore(cast<Instruction>(NewVal), *I);
// Then cast that to the destination type.
NewVal = new CastInst(NewVal, I->getType(), I->getName());
InsertNewInstBefore(cast<Instruction>(NewVal), *I);
There is no reason to turn an ashr into two casts + a lshr: just turn
it into a lshr instead.
In this hunk:
@@ -1897,33 +1912,46 @@ Instruction *InstCombiner::visitSub(Bina
...
the two copies of:
if (NewShift->getType() == I.getType())
return NewShift;
else {
InsertNewInstBefore(NewShift, I);
return new CastInst(NewShift, I.getType());
}
Can be turned into:
return NewShift;
This allows you to simplify them into code like this (likewise for
the LShr case):
+ else if (SI->getOpcode() == Instruction::AShr) {
+ if (ConstantInt *CU = dyn_cast<ConstantInt>(SI->getOperand
(1))) {
+ // Check to see if we are shifting out everything but
the sign bit.
+ if (CU->getZExtValue() ==
+ SI->getType()->getPrimitiveSizeInBits()-1) {
+ // Ok, the transformation is safe. Insert LShr.
+ return new ShiftInst(Instruction::LShr, SI->getOperand
(0),
+ CU, SI->getName());
+ }
+ }
+ }
In this hunk:
@@ -2260,22 +2288,14 @@ Instruction *InstCombiner::visitUDiv(Bin
Please simplify the code to just "return new ShiftInst(...)",
eliminating the Result variable. This occurs many times, please
simplify them all.
In these chunks:
@@ -2805,18 +2808,19 @@ Instruction *InstCombiner::OptAndOp(Inst
@@ -2826,19 +2830,19 @@ Instruction *InstCombiner::OptAndOp(Inst
You shouldn't be checking 'AndRHS->getType()->isUnsigned()' to
determine if it's a LShr or AShr. This is a bug.
In the same chunks, there are also unneeded casts being inserted,
please remove them.
In this hunk:
@@ -5086,12 +5096,14 @@ Instruction *InstCombiner::FoldShiftByCo
case Instruction::Or:
case Instruction::Xor:
// These operators commute.
// Turn (Y + (X >> C)) << C -> (X + (Y << C)) & (~0 << C)
if (isLeftShift && Op0BO->getOperand(1)->hasOneUse() &&
- match(Op0BO->getOperand(1),
- m_Shr(m_Value(V1), m_ConstantInt(CC))) && CC ==
Op1) {
+ (match(Op0BO->getOperand(1),
+ m_LShr(m_Value(V1), m_ConstantInt(CC))) ||
+ match(Op0BO->getOperand(1),
+ m_AShr(m_Value(V1), m_ConstantInt(CC)))) && CC
== Op1) {
It seems that it would be better to introduce a new 'm_Shr' predicate
that matches either shift right operation.
Likewise in:
@@ -5103,13 +5115,16 @@ Instruction *InstCombiner::FoldShiftByCo
@@ -5123,12 +5138,14 @@ Instruction *InstCombiner::FoldShiftByCo
@@ -5140,13 +5157,16 @@ Instruction *InstCombiner::FoldShiftByCo
etc.
There is still unneeded casting happening in:
@@ -5282,14 +5302,14 @@ Instruction *InstCombiner::FoldShiftByCo
return new ShiftInst(I.getOpcode(), Mask,
ConstantInt::get(Type::UByteTy, ShiftAmt2-
ShiftAmt1));
} else if (isShiftOfUnsignedShift || isShiftOfLeftShift) {
if (isShiftOfUnsignedShift && !isShiftOfLeftShift &&
isSignedShift) {
// Make sure to emit an unsigned shift right, not a signed
one.
- Mask = InsertNewInstBefore(new CastInst(Mask,
+ Mask = InsertNewInstBefore(new CastInst(Mask,
Mask->getType()-
>getUnsignedVersion(),
Op->getName()), I);
- Mask = new ShiftInst(Instruction::Shr, Mask,
+ Mask = new ShiftInst(Instruction::LShr, Mask,
ConstantInt::get(Type::UByteTy, ShiftAmt1-
ShiftAmt2));
InsertNewInstBefore(Mask, I);
return new CastInst(Mask, I.getType());
} else {
return new ShiftInst(ShiftOp->getOpcode(), Mask,
In this chunk:
@@ -5798,23 +5818,25 @@ Instruction *InstCombiner::visitCastInst
(DestBitSize < SrcBitSize && isa<Constant>(Op1))) {
Value *Op0c = InsertOperandCastBefore(Op0, DestTy, SrcI);
return new ShiftInst(Instruction::Shl, Op0c, Op1);
}
break;
- case Instruction::Shr:
+ case Instruction::LShr:
+ break;
+ case Instruction::AShr:
// If this is a signed shr, and if all bits shifted in are
about to be
// truncated off, turn it into an unsigned shr to allow greater
// simplifications.
- if (DestBitSize < SrcBitSize && Src->getType()->isSigned() &&
+ if (DestBitSize < SrcBitSize &&
isa<ConstantInt>(Op1)) {
unsigned ShiftAmt = cast<ConstantInt>(Op1)->getZExtValue();
if (SrcBitSize > ShiftAmt && SrcBitSize-ShiftAmt >=
DestBitSize) {
// Convert to unsigned.
Value *N1 = InsertOperandCastBefore(Op0,
Op0->getType()-
>getUnsignedVersion(), &CI);
// Insert the new shift, which is now unsigned.
- N1 = InsertNewInstBefore(new ShiftInst(Instruction::Shr,
N1,
+ N1 = InsertNewInstBefore(new ShiftInst
(Instruction::LShr, N1,
Op1, Src->getName
()), CI);
return new CastInst(N1, CI.getType());
}
}
break;
...
Please remove:
"
+ case Instruction::LShr:
+ break;
"
And eliminate the casting.
In this chunk:
@@ -5857,11 +5879,11 @@ Instruction *InstCombiner::visitCastInst
// unsigned if it is signed.
if (In->getType()->isSigned())
In = InsertCastBefore(
In, In->getType()->getUnsignedVersion(), CI);
// Insert the shift to put the result in the low bit.
- In = InsertNewInstBefore(new ShiftInst
(Instruction::Shr, In,
+ In = InsertNewInstBefore(new ShiftInst
(Instruction::LShr, In,
ConstantInt::get(Type::UByteTy,
ShiftAmt),
In->getName()+".lobit"), CI);
}
if ((Op1CV != 0) == isSetNE) { // Toggle the low bit.
Remove the casting.
Please resend with the appropriate modifications, thanks.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061105/47754ced/attachment.html>
More information about the llvm-commits
mailing list