[llvm-commits] [llvm] r122950 - in /llvm/trunk: lib/Analysis/ConstantFolding.cpp test/Transforms/InstCombine/constant-fold-gep.ll

[Chris Lattner]

On Jan 6, 2011, at 9:16 AM, Duncan Sands wrote:

> Hi Chris,
> 
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/ConstantFolding.cpp (original)
>> +++ llvm/trunk/lib/Analysis/ConstantFolding.cpp Thu Jan  6 00:19:46 2011
>> @@ -575,8 +575,26 @@
>>    // If this is a constant expr gep that is effectively computing an
>>    // "offsetof", fold it into 'cast int Size to T*' instead of 'gep 0, 0, 12'
>>    for (unsigned i = 1; i != NumOps; ++i)
>> -    if (!isa<ConstantInt>(Ops[i]))
>> +    if (!isa<ConstantInt>(Ops[i])) {
>> +
>> +      // If this is "gep i8* Ptr, (sub 0, V)", fold this as:
>> +      // "inttoptr (sub (ptrtoint Ptr), V)"
>> +      if (NumOps == 2&&
>> +          cast<PointerType>(ResultTy)->getElementType()->isIntegerTy(8)) {
> 
> you could do this for other pointer types by introducing a multiplication.
> But perhaps it isn't interesting?

The idiom I was looking at (std::fill on arrays) was able to compute a trip count for all cases other than when the array was of bytes... so I assume that there is another canonicalization kicking in.

> 
>> +        ConstantExpr *CE = dyn_cast<ConstantExpr>(Ops[1]);
>> +        if (CE&&  CE->getOpcode() == Instruction::Sub&&
>> +            isa<ConstantInt>(CE->getOperand(0))&&
>> +            cast<ConstantInt>(CE->getOperand(0))->isZero()) {
> 
> The above two lines could be turned into: CE->getOperand(0)->isNullValue()

Nice cleanup, done.

>> +          Constant *Res = ConstantExpr::getPtrToInt(Ptr, CE->getType());
> 
> Here you convert the pointer to an integer of the type of the sub.  I don't
> think that makes sense, since can't the GEP index have any type?  For example,
> suppose the type of the sub was i8, then here you convert the pointer to an i8
> which is wrong.

This code is in SymbolicallyEvaluateGEP, which only is run if TD is available.  SymbolicallyEvaluateGEP is run only after CastGEPIndices, which canonicalizes the indexes to intptr_t.  I agree that this is subtle so I added an assert to make it clear.

Thanks for the review!  llvm.org seems to be missing, but i'll commit the patch when it's back.

-Chris