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

Chris Lattner clattner at apple.com
Tue Dec 12 23:14:55 PST 2006


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.

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

> @@ -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?

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

-Chris




More information about the llvm-commits mailing list