[llvm-commits] CVS: llvm/lib/Transforms/Scalar/InstructionCombining.cpp

[Reid Spencer]

Hi Chris,

Feedback below ...

On Tue, 2006-12-12 at 23:14 -0800, Chris Lattner wrote:
> On Dec 12, 2006, at 3:36 PM, Reid Spencer wrote:
> > @@ -3286,7 +3281,8 @@
> >                                                         Op1C- 
> > >getOperand(0),
> >                                                         I.getName());
> >          InsertNewInstBefore(NewOp, I);
> > -        return CastInst::createInferredCast(NewOp, I.getType());
> > +        return CastInst::createIntegerCast(NewOp, I.getType(),
> > +                                           SrcTy->isSigned());
> >        }
> >      }
> >    }
> > @@ -3690,7 +3686,8 @@
> >                                                        Op1C- 
> > >getOperand(0),
> >                                                        I.getName());
> >          InsertNewInstBefore(NewOp, I);
> > -        return CastInst::createInferredCast(NewOp, I.getType());
> > +        return CastInst::createIntegerCast(NewOp, I.getType(),
> > +                                           SrcTy->isSigned());
> >        }
> >    }
> >
> > @@ -3871,7 +3868,8 @@
> >                                                         Op1C- 
> > >getOperand(0),
> >                                                         I.getName());
> >          InsertNewInstBefore(NewOp, I);
> > -        return CastInst::createInferredCast(NewOp, I.getType());
> > +        return CastInst::createIntegerCast(NewOp, I.getType(),
> > +                                           SrcTy->isSigned());
> >        }
> >    }
> 
> These three all point out serious bugs.  The code looks like this for  
> each:
> 
>    // fold (and (cast A), (cast B)) -> (cast (and A, B))
>    if (CastInst *Op1C = dyn_cast<CastInst>(Op1)) {
>      if (CastInst *Op0C = dyn_cast<CastInst>(Op0)) {
>        const Type *SrcTy = Op0C->getOperand(0)->getType();
>        if (SrcTy == Op1C->getOperand(0)->getType() && SrcTy- 
>  >isIntegral() &&
>            // Only do this if the casts both really cause code to be  
> generated.
>            ValueRequiresCast(Op0C->getOperand(0), I.getType(), TD) &&
>            ValueRequiresCast(Op1C->getOperand(0), I.getType(), TD)) {
>          Instruction *NewOp = BinaryOperator::createAnd(Op0C- 
>  >getOperand(0),
>                                                         Op1C- 
>  >getOperand(0),
>                                                         I.getName());
>          InsertNewInstBefore(NewOp, I);
>          return CastInst::createIntegerCast(NewOp, I.getType(),
>                                             SrcTy->isSigned());
>        }
>      }
>    }
> 
> This xform used to be safe before the cast patch, but now can turn  
> stuff like:
> 
>    (and (sext A), (zext B))
> 
> into:
> 
>    (sext (and A, B))
> 
> which is very wrong (again, never use SrcTy->isSigned!).
> 
> All three of these cases should verify that Op1C->getOpcode() == Op2C- 
>  >getOpcode(), and it should use the opcode in the inserted cast.   
> That is, either of these are allowed:
> 
>    (and (sext A), (sext B)) -> (sext (and A, B))
>    (and (zext A), (zext B)) -> (zext (and A, B))
> 
> but no mixing.

Yup, you're right. This is bad. I committed this fix separately last
night.

> 
> > @@ -5392,8 +5389,7 @@
> >
> >        Value *Op = ShiftOp->getOperand(0);
> >        if (isShiftOfSignedShift != isSignedShift)
> > -        Op = InsertNewInstBefore(
> > -               CastInst::createInferredCast(Op, I.getType(),  
> > "tmp"), I);
> > +        Op = InsertNewInstBefore(new BitCastInst(Op, I.getType(),  
> > "tmp"), I);
> 
> This cast can be removed, now that shifts are signless.

Yup. Done.

> 
> > @@ -5681,7 +5677,7 @@
> >  /// evaluate the expression.
> >  Value *InstCombiner::EvaluateInDifferentType(Value *V, const Type  
> > *Ty) {
> >    if (Constant *C = dyn_cast<Constant>(V))
> > -    return ConstantExpr::getCast(C, Ty);
> > +    return ConstantExpr::getIntegerCast(C, Ty, C->getType()- 
> > >isSigned());
> 
> This looks extremely unsafe.  Why is it ok?

The only place EvaluateInDifferentType is called is from
commonIntCastTransforms. Those casts are guaranteed to only involve
integer operands. Therefore, the only kind of cast that could be
involved is an integer cast.

What looks "extremely" unsafe about it?

> 
> > @@ -7950,13 +7943,20 @@
> >          // the same size.  Instead of casting the pointer before  
> > the store, cast
> >          // the value to be stored.
> >          Value *NewCast;
> > -        if (Constant *C = dyn_cast<Constant>(SI.getOperand(0)))
> > -          NewCast = ConstantExpr::getCast(C, SrcPTy);
> > +        Instruction::CastOps opcode = Instruction::BitCast;
> > +        Value *SIOp0 = SI.getOperand(0);
> > +        if (SrcPTy->getTypeID() == Type::PointerTyID) {
> 
> isa<PointerType>(SrcPTy)
> 
> > +          if (SIOp0->getType()->isIntegral())
> > +            opcode = Instruction::IntToPtr;
> > +        } else if (SrcPTy->isIntegral()) {
> > +          if (SIOp0->getType()->getTypeID() == Type::PointerTyID)
> 
> isa<PointerType>

Both done.

> 
> -Chris
>