[llvm-commits] [llvm] r162120 - in /llvm/trunk: include/llvm/Analysis/MemoryBuiltins.h lib/Analysis/MemoryBuiltins.cpp test/Transforms/InstCombine/objsize.ll

[Benjamin Kramer]

On 17.08.2012, at 21:39, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Fri, Aug 17, 2012 at 12:26 PM, Benjamin Kramer
> <benny.kra at googlemail.com> wrote:
>> Author: d0k
>> Date: Fri Aug 17 14:26:41 2012
>> New Revision: 162120
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=162120&view=rev
>> Log:
>> MemoryBuiltins: Properly guard ObjectSizeOffsetVisitor against cycles in the IR.
>> 
>> The previous fix only checked for simple cycles, use a set to catch longer
>> cycles too.
>> 
>> Drop the broken check from the ObjectSizeOffsetEvaluator. The BoundsChecking
>> pass doesn't have to deal with invalid IR like InstCombine does.
>> 
>> Modified:
>>    llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h
>>    llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
>>    llvm/trunk/test/Transforms/InstCombine/objsize.ll
>> 
>> Modified: llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h?rev=162120&r1=162119&r2=162120&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h (original)
>> +++ llvm/trunk/include/llvm/Analysis/MemoryBuiltins.h Fri Aug 17 14:26:41 2012
>> @@ -146,6 +146,7 @@
>>   bool RoundToAlign;
>>   unsigned IntTyBits;
>>   APInt Zero;
>> +  SmallPtrSet<Instruction *, 8> SeenInsts;
>> 
>>   APInt align(APInt Size, uint64_t Align);
>> 
>> @@ -203,7 +204,6 @@
>>   const TargetData *TD;
>>   LLVMContext &Context;
>>   BuilderTy Builder;
>> -  ObjectSizeOffsetVisitor Visitor;
>>   IntegerType *IntTy;
>>   Value *Zero;
>>   CacheMapTy CacheMap;
>> 
>> Modified: llvm/trunk/lib/Analysis/MemoryBuiltins.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryBuiltins.cpp?rev=162120&r1=162119&r2=162120&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/MemoryBuiltins.cpp (original)
>> +++ llvm/trunk/lib/Analysis/MemoryBuiltins.cpp Fri Aug 17 14:26:41 2012
>> @@ -358,11 +358,16 @@
>> 
>> SizeOffsetType ObjectSizeOffsetVisitor::compute(Value *V) {
>>   V = V->stripPointerCasts();
>> +  if (Instruction *I = dyn_cast<Instruction>(V)) {
>> +    // If we have already seen this instruction, bail out. Cycles can happen in
>> +    // unreachable code after constant propagation.
>> +    if (!SeenInsts.insert(I))
>> +      return unknown();
>> 
>> -  if (GEPOperator *GEP = dyn_cast<GEPOperator>(V))
>> -    return visitGEPOperator(*GEP);
>> -  if (Instruction *I = dyn_cast<Instruction>(V))
>> +    if (GEPOperator *GEP = dyn_cast<GEPOperator>(V))
>> +      return visitGEPOperator(*GEP);
>>     return visit(*I);
>> +  }
>>   if (Argument *A = dyn_cast<Argument>(V))
>>     return visitArgument(*A);
>>   if (ConstantPointerNull *P = dyn_cast<ConstantPointerNull>(V))
>> @@ -371,9 +376,12 @@
>>     return visitGlobalVariable(*GV);
>>   if (UndefValue *UV = dyn_cast<UndefValue>(V))
>>     return visitUndefValue(*UV);
>> -  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V))
>> +  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(V)) {
>>     if (CE->getOpcode() == Instruction::IntToPtr)
>>       return unknown(); // clueless
>> +    if (CE->getOpcode() == Instruction::GetElementPtr)
>> +      return visitGEPOperator(cast<GEPOperator>(*CE));
>> +  }
>> 
>>   DEBUG(dbgs() << "ObjectSizeOffsetVisitor::compute() unhandled value: " << *V
>>         << '\n');
>> @@ -473,10 +481,6 @@
>> }
>> 
>> SizeOffsetType ObjectSizeOffsetVisitor::visitGEPOperator(GEPOperator &GEP) {
>> -  // Ignore self-referencing GEPs, they can occur in unreachable code.
>> -  if (&GEP == GEP.getPointerOperand())
>> -    return unknown();
>> -
>>   SizeOffsetType PtrData = compute(GEP.getPointerOperand());
>>   if (!bothKnown(PtrData) || !GEP.hasAllConstantIndices())
>>     return unknown();
>> @@ -510,10 +514,6 @@
>> }
>> 
>> SizeOffsetType ObjectSizeOffsetVisitor::visitSelectInst(SelectInst &I) {
>> -  // ignore malformed self-looping selects
>> -  if (I.getTrueValue() == &I || I.getFalseValue() == &I)
>> -    return unknown();
>> -
>>   SizeOffsetType TrueSide  = compute(I.getTrueValue());
>>   SizeOffsetType FalseSide = compute(I.getFalseValue());
>>   if (bothKnown(TrueSide) && bothKnown(FalseSide) && TrueSide == FalseSide)
>> @@ -533,8 +533,7 @@
>> 
>> ObjectSizeOffsetEvaluator::ObjectSizeOffsetEvaluator(const TargetData *TD,
>>                                                      LLVMContext &Context)
>> -: TD(TD), Context(Context), Builder(Context, TargetFolder(TD)),
>> -Visitor(TD, Context) {
>> +: TD(TD), Context(Context), Builder(Context, TargetFolder(TD)) {
>>   IntTy = TD->getIntPtrType(Context);
>>   Zero = ConstantInt::get(IntTy, 0);
>> }
>> @@ -559,6 +558,7 @@
>> }
>> 
>> SizeOffsetEvalType ObjectSizeOffsetEvaluator::compute_(Value *V) {
>> +  ObjectSizeOffsetVisitor Visitor(TD, Context);
>>   SizeOffsetType Const = Visitor.compute(V);
>>   if (Visitor.bothKnown(Const))
>>     return std::make_pair(ConstantInt::get(Context, Const.first),
>> @@ -719,10 +719,6 @@
>> }
>> 
>> SizeOffsetEvalType ObjectSizeOffsetEvaluator::visitSelectInst(SelectInst &I) {
>> -  // ignore malformed self-looping selects
>> -  if (I.getTrueValue() == &I || I.getFalseValue() == &I)
>> -    return unknown();
>> -
>>   SizeOffsetEvalType TrueSide  = compute_(I.getTrueValue());
>>   SizeOffsetEvalType FalseSide = compute_(I.getFalseValue());
>> 
>> 
>> Modified: llvm/trunk/test/Transforms/InstCombine/objsize.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/objsize.ll?rev=162120&r1=162119&r2=162120&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/Transforms/InstCombine/objsize.ll (original)
>> +++ llvm/trunk/test/Transforms/InstCombine/objsize.ll Fri Aug 17 14:26:41 2012
>> @@ -247,7 +247,8 @@
>> 
>> ; technically reachable, but this malformed IR may appear as a result of constant propagation
>> xpto:
>> -  %gep = getelementptr i8* %gep, i32 1
>> +  %gep2 = getelementptr i8* %gep, i32 1
>> +  %gep = getelementptr i8* %gep2, i32 1
>>   %o = call i32 @llvm.objectsize.i32(i8* %gep, i1 true)
>> ; CHECK: ret i32 undef
>>   ret i32 %o
> 
> Err, wait a sec: this testcase isn't valid!  The verifier should catch it.

The verifier only runs after the specified pass, instcombine removes dead code so it doesn't fire. It's really hard to find a test case for this without writing invalid IR, and if you have a reduced one it tends to be fragile :(

- Ben