[llvm-commits] SHR Patch (For Review)

[Reid Spencer]

On Tue, 2006-11-07 at 19:58 -0800, Chris Lattner wrote:
> 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).
> 

Found 3, only in InstCombine

> 
> 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.

That's the one that causes InstCombiner's replaceAllUsesWith call to
assert because its the wrong type.  This is specifically the one I
warned you about :)

If you can see a way to eliminate it, please feel free after I commit
it. We already tried and it breaks.

> 
> 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.

Right. Done.

> 
> 
> 
> 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.

Done.

> 
> 
> 
> 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,

Will do immediately after the commit.

Thanks Chris!