[llvm-commits] [llvm] r156585 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombine.h lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstCombineCalls.cpp test/Transforms/InstCombine/objsize.ll

Tue May 15 17:21:35 PDT 2012

>>>> objectsize: add support for GEPs with non-constant indexes
>>>> add an additional parameter to InstCombiner::EmitGEPOffset() to  
>>>> force it to
>>>> *not* emit operations with NUW flag
>>>
>>> why? It seems strange - can you please explain.
>>
>> Sure!
>> The idea is that even if a GEP has the inbounds flag, we want to have
>> well-defined semantics for the computation of the offset. Otherwise  
>> the bounds
>> check could be eliminated because of the undefinedness of NUW on overflow.
>> Basically this parameter makes EmitGEPOffset ignore the inbounds attribute.
>
> thanks for explaining.  But will it actually work?  The GEP indices  
> themselves
> could already be a computation with nuw/nsw flags or be undef, which could
> propagate through.  All kinds of transforms might already have munched on the
> GEPs, exploiting "inbounds" to transform them to something "wrong" (from your
> point of view).  Also, the name NoNUW could be better I reckon.  I mean: it's
> not nuw wrap you really care about, you would also care about nsw or undef or
> who knows what.  How about NoAssumptions (though I don't actually like this
> either)?  In fact, since you have special needs I can't help feeling that you
> should write your own routines and not make use of generic routines  
> like this.

Ah, I see your point.
I guess you're right. Maybe I should just avoid adding inbounds in  
clang in the first place (when doing bounds checking only, of course).
Let me give a further thought on this and discuss with people around..

Thanks for your review!
Nuno

> Ciao, Duncan.
>
>>
>> Nuno
>>
>>
>>> Ciao, Duncan.
>>>
>>>> Modified:
>>>> llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
>>>> llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>>> llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>>> llvm/trunk/test/Transforms/InstCombine/objsize.ll
>>>>
>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombine.h
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombine.h?rev=156585&r1=156584&r2=156585&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombine.h (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombine.h Thu May  
>>>> 10 18:17:35 2012
>>>> @@ -226,7 +226,7 @@
>>>> bool DoXform = true);
>>>> Instruction *transformSExtICmp(ICmpInst *ICI, Instruction&CI);
>>>> bool WillNotOverflowSignedAdd(Value *LHS, Value *RHS);
>>>> - Value *EmitGEPOffset(User *GEP);
>>>> + Value *EmitGEPOffset(User *GEP, bool NoNUW = false);
>>>>
>>>> public:
>>>> // InsertNewInstBefore - insert an instruction New before instruction Old
>>>>
>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp?rev=156585&r1=156584&r2=156585&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp Thu May 10
>>>> 18:17:35 2012
>>>> @@ -423,7 +423,8 @@
>>>> /// EmitGEPOffset - Given a getelementptr  
>>>> instruction/constantexpr, emit the
>>>> /// code necessary to compute the offset from the base pointer  
>>>> (without adding
>>>> /// in the base pointer). Return the result as a signed integer  
>>>> of intptr size.
>>>> -Value *InstCombiner::EmitGEPOffset(User *GEP) {
>>>> +/// If NoNUW is true, then the NUW flag is not used.
>>>> +Value *InstCombiner::EmitGEPOffset(User *GEP, bool NoNUW) {
>>>> TargetData&TD = *getTargetData();
>>>> gep_type_iterator GTI = gep_type_begin(GEP);
>>>> Type *IntPtrTy = TD.getIntPtrType(GEP->getContext());
>>>> @@ -431,7 +432,7 @@
>>>>
>>>> // If the GEP is inbounds, we know that none of the addressing  
>>>> operations will
>>>> // overflow in an unsigned sense.
>>>> - bool isInBounds = cast<GEPOperator>(GEP)->isInBounds();
>>>> + bool isInBounds = cast<GEPOperator>(GEP)->isInBounds()&& !NoNUW;
>>>>
>>>> // Build a mask for high order bits.
>>>> unsigned IntPtrWidth = TD.getPointerSizeInBits();
>>>>
>>>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=156585&r1=156584&r2=156585&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
>>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Thu May 10
>>>> 18:17:35 2012
>>>> @@ -173,7 +173,7 @@
>>>> uint64_t Penalty, TargetData *TD,
>>>> InstCombiner::BuilderTy *Builder) {
>>>> if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Alloc)) {
>>>> - if (GV->hasUniqueInitializer()) {
>>>> + if (GV->hasDefinitiveInitializer()) {
>>>> Constant *C = GV->getInitializer();
>>>> Size = TD->getTypeAllocSize(C->getType());
>>>> return 1;
>>>> @@ -198,7 +198,8 @@
>>>> if (Penalty< 2)
>>>> return 2;
>>>>
>>>> - SizeValue = Builder->CreateMul(Builder->getInt64(Size), ArraySize);
>>>> + SizeValue = ConstantInt::get(ArraySize->getType(), Size);
>>>> + SizeValue = Builder->CreateMul(SizeValue, ArraySize);
>>>> return 0;
>>>>
>>>> } else if (CallInst *MI = extractMallocCall(Alloc)) {
>>>> @@ -320,22 +321,12 @@
>>>>
>>>> // Get to the real allocated thing and offset as fast as possible.
>>>> Value *Op1 = II->getArgOperand(0)->stripPointerCasts();
>>>> + GEPOperator *GEP;
>>>>
>>>> - uint64_t Offset = 0;
>>>> - Value *OffsetValue;
>>>> - bool ConstOffset = true;
>>>> -
>>>> - // Try to look through constant GEPs.
>>>> - if (GEPOperator *GEP = dyn_cast<GEPOperator>(Op1)) {
>>>> - if (!GEP->hasAllConstantIndices()) return 0;
>>>> -
>>>> - // Get the current byte offset into the thing. Use the original
>>>> - // operand in case we're looking through a bitcast.
>>>> - SmallVector<Value*, 8> Ops(GEP->idx_begin(), GEP->idx_end());
>>>> - if (!GEP->getPointerOperandType()->isPointerTy())
>>>> + if ((GEP = dyn_cast<GEPOperator>(Op1))) {
>>>> + // check if we will be able to get the offset
>>>> + if (!GEP->hasAllConstantIndices()&& Penalty< 2)
>>>> return 0;
>>>> - Offset = TD->getIndexedOffset(GEP->getPointerOperandType(), Ops);
>>>> -
>>>> Op1 = GEP->getPointerOperand()->stripPointerCasts();
>>>> }
>>>>
>>>> @@ -349,28 +340,36 @@
>>>> if (ConstAlloc == 2)
>>>> return 0;
>>>>
>>>> - if (ConstOffset&& ConstAlloc) {
>>>> + uint64_t Offset = 0;
>>>> + Value *OffsetValue = 0;
>>>> +
>>>> + if (GEP) {
>>>> + if (GEP->hasAllConstantIndices()) {
>>>> + SmallVector<Value*, 8> Ops(GEP->idx_begin(), GEP->idx_end());
>>>> + assert(GEP->getPointerOperandType()->isPointerTy());
>>>> + Offset = TD->getIndexedOffset(GEP->getPointerOperandType(), Ops);
>>>> + } else
>>>> + OffsetValue = EmitGEPOffset(GEP, true /*NoNUW*/);
>>>> + }
>>>> +
>>>> + if (!OffsetValue&& ConstAlloc) {
>>>> if (Size< Offset) {
>>>> // Out of bounds
>>>> return ReplaceInstUsesWith(CI, ConstantInt::get(ReturnTy, 0));
>>>> }
>>>> return ReplaceInstUsesWith(CI, ConstantInt::get(ReturnTy, Size-Offset));
>>>> + }
>>>>
>>>> - } else if (Penalty>= 2) {
>>>> - if (ConstOffset)
>>>> - OffsetValue = Builder->getInt64(Offset);
>>>> - if (ConstAlloc)
>>>> - SizeValue = Builder->getInt64(Size);
>>>> -
>>>> - Value *Val = Builder->CreateSub(SizeValue, OffsetValue);
>>>> - Val = Builder->CreateTrunc(Val, ReturnTy);
>>>> - // return 0 if there's an overflow
>>>> - Value *Cmp = Builder->CreateICmpULT(SizeValue, OffsetValue);
>>>> - Val = Builder->CreateSelect(Cmp, ConstantInt::get(ReturnTy, 0), Val);
>>>> - return ReplaceInstUsesWith(CI, Val);
>>>> -
>>>> - } else
>>>> - return 0;
>>>> + if (!OffsetValue)
>>>> + OffsetValue = ConstantInt::get(ReturnTy, Offset);
>>>> + if (ConstAlloc)
>>>> + SizeValue = ConstantInt::get(ReturnTy, Size);
>>>> +
>>>> + Value *Val = Builder->CreateSub(SizeValue, OffsetValue);
>>>> + // return 0 if there's an overflow
>>>> + Value *Cmp = Builder->CreateICmpULT(SizeValue, OffsetValue);
>>>> + Val = Builder->CreateSelect(Cmp, ConstantInt::get(ReturnTy, 0), Val);
>>>> + return ReplaceInstUsesWith(CI, Val);
>>>> }
>>>> case Intrinsic::bswap:
>>>> // bswap(bswap(x)) -> x
>>>>
>>>> Modified: llvm/trunk/test/Transforms/InstCombine/objsize.ll
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/objsize.ll?rev=156585&r1=156584&r2=156585&view=diff
>>>>
>>>> ==============================================================================
>>>> --- llvm/trunk/test/Transforms/InstCombine/objsize.ll (original)
>>>> +++ llvm/trunk/test/Transforms/InstCombine/objsize.ll Thu May 10  
>>>> 18:17:35 2012
>>>> @@ -168,3 +168,28 @@
>>>> ; CHECK-NEXT: ret i32 30
>>>> ret i32 %objsize
>>>> }
>>>> +
>>>> +; CHECK: @test9
>>>> +define i32 @test9(i32 %x, i32 %y) nounwind {
>>>> + %a = alloca [3 x [4 x double]], align 8
>>>> + %1 = getelementptr inbounds [3 x [4 x double]]* %a, i32 0, i32 %x
>>>> + %2 = getelementptr inbounds [4 x double]* %1, i32 0, i32 %y
>>>> + %3 = bitcast double* %2 to i8*
>>>> + %objsize = call i32 @llvm.objectsize.i32(i8* %3, i1 false, i32 2)
>>>> + ret i32 %objsize
>>>> +; CHECK-NEXT: shl i32 %x, 5
>>>> +; CHECK-NEXT: shl i32 %y, 3
>>>> +; CHECK-NEXT: add i32
>>>> +; CHECK-NEXT: sub i32 96,
>>>> +; CHECK-NEXT: icmp ugt i32 {{.*}}, 96
>>>> +; CHECK-NEXT: select i1 {{.*}}, i32 0,
>>>> +}
>>>> +
>>>> +; CHECK: @overflow
>>>> +define i32 @overflow() {
>>>> + %alloc = call noalias i8* @malloc(i32 21) nounwind
>>>> + %gep = getelementptr inbounds i8* %alloc, i32 50
>>>> + %objsize = call i32 @llvm.objectsize.i32(i8* %gep, i1 false, i32 0)
>>>> nounwind readonly
>>>> +; CHECK-NEXT: ret i32 0
>>>> + ret i32 %objsize
>>>> +}