[llvm-commits] SHR Patch (For Review)

[Chris Lattner]

On Nov 7, 2006, at 5:33 PM, Reid Spencer wrote:

> Attached is the 3rd attempt to get the SHR patch right. This one  
> passes
> all the tests and eliminates many more casts than previous versions.
>
> NOTE: Please don't commit this!
>
> Reid.

Overall, the patch looks excellent.  There is some minor  
typographical cruft like "return  new ShiftInst" in various places.   
Please grep for that (turning two spaces before new into one).



In this hunk:

@@ -5250,12 +5230,16 @@ Instruction *InstCombiner::FoldShiftByCo
          Amt = Op0->getType()->getPrimitiveSizeInBits();

        Value *Op = ShiftOp->getOperand(0);
        if (isShiftOfSignedShift != isSignedShift)
          Op = InsertNewInstBefore(new CastInst(Op, I.getType(),  
"tmp"), I);
-      return new ShiftInst(I.getOpcode(), Op,
+      ShiftInst* ShiftResult = new ShiftInst(I.getOpcode(), Op,
                             ConstantInt::get(Type::UByteTy, Amt));
+      if (I.getType() == ShiftResult->getType())
+        return ShiftResult;
+      InsertNewInstBefore(ShiftResult, I);
+      return new CastInst(ShiftResult, I.getType());
      }

Why do you need the two casts?  If you don't, please remove them.




In this hunk:
@@ -5790,33 +5768,28 @@ Instruction *InstCombiner::visitCastInst
...
-            // Insert the new shift, which is now unsigned.
-            N1 = InsertNewInstBefore(new ShiftInst(Instruction::Shr,  
N1,
-                                                   Op1, Src->getName 
()), CI);
-            return new CastInst(N1, CI.getType());
+            // Insert the new logical shift right.
+            return new ShiftInst(Instruction::LShr, Op0, Op1, Src- 
 >getName());

You shouldn't pass Src->getName() to the instruction ctor.




RCS file: /var/cvs/llvm/llvm/lib/VMCore/Instructions.cpp,v
retrieving revision 1.45
diff -t -d -u -p -5 -r1.45 Instructions.cpp
--- lib/VMCore/Instructions.cpp	2 Nov 2006 01:53:58 -0000	1.45
+++ lib/VMCore/Instructions.cpp	8 Nov 2006 00:31:58 -0000
@@ -1228,11 +1228,11 @@ bool BinaryOperator::swapOperands() {
  // 
===--------------------------------------------------------------------- 
-===//

  /// isLogicalShift - Return true if this is a logical shift left or  
a logical
  /// shift right.
  bool ShiftInst::isLogicalShift() const {
-  return getOpcode() == Instruction::Shl || getType()->isUnsigned();
+  return getOpcode() == Instruction::Shl || getOpcode() ==  
Instruction::LShr;
  }

isLogicalShift can now be an inline method in the header for  
ShiftInst, now that it isn't touching getType().  Please move it to  
the header.




After making the changes above and retesting, please commit this!   
After this goes in, please remember to do the next patch which turns  
ConstantExpr::getUShr -> ConstantExpr::getLShr etc.  Thanks guys,

-Chris