[llvm-commits] [llvm] r157261 - in /llvm/trunk: include/llvm/InitializePasses.h include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h include/llvm/Transforms/Utils/Local.h lib/Transforms/InstCombine/InstCombine.h lib/Transforms/InstCombine/InstCombineAddSub.cpp lib/Transforms/InstCombine/InstructionCombining.cpp lib/Transforms/Scalar/BoundsChecking.cpp lib/Transforms/Scalar/Scalar.cpp

[Nuno Lopes]

Hi John,

Thanks for the throughout review!

Some replies inline:


>> URL: http://llvm.org/viewvc/llvm-project?rev=157261&view=rev
>> Log:
>> add a new pass to instrument loads and stores for run-time bounds checking
>> move EmitGEPOffset from InstCombine to Transforms/Utils/Local.h
>>
> Have you measured the performance overhead of the code when this  
> pass is used?

No, not yet.
This pass isn't ready by any means. This is just a first shot; I'll be  
improving it over the next few weeks.
I'll send around some numbers as soon as I have them.


>> +ConstTriState BoundsChecking::computeAllocSize(Value *Alloc, uint64_t&Size,
>> +                                     Value*&SizeValue) {
>> +  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Alloc)) {
>> +    if (GV->hasDefinitiveInitializer()) {
>> +      Constant *C = GV->getInitializer();
>> +      Size = TD->getTypeAllocSize(C->getType());
>> +      return Const;
>> +    }
>> +    return Dunno;
>
> You can find the size of a global even if it doesn't have an  
> initializer; the trick is to make sure that the global is not  
> declared in another compilation unit.

uhm, if the global doesn't have a definitive initializer, how can you  
be sure that it won't be replaced in, e.g., link time?  Unless I'm  
missing something, I think this is correct.


>> +  // Get to the real allocated thing and offset as fast as possible.
>> +  Ptr = Ptr->stripPointerCasts();
>> +  GEPOperator *GEP;
>> +
>> +  if ((GEP = dyn_cast<GEPOperator>(Ptr))) {
>> +    // check if we will be able to get the offset
>> +    if (!GEP->hasAllConstantIndices()&&  Penalty<  2) {
>> +      ++ChecksUnable;
>> +      return false;
>> +    }
>> +    Ptr = GEP->getPointerOperand()->stripPointerCasts();
>> +  }
>
> If I understand this code correctly, you're just checking to see if  
> the dereferenced pointer (Ptr) is a GEP and, if so, generating code  
> to calculate it's offset.  What if Ptr is a PHI node derived from a  
> GEP?  I imagine you'd see that in loops a lot.  Does it handle  
> non-pointer casts (like inttoptr and ptrtoint, which I think some of  
> the loop transforms add when modifying GEPs)?  Unless I'm not seeing  
> something, your code will miss a lot of trivial cases.  Is this  
> intentional or an oversight?

Right now the code doesn't support PHIs nor selects, but support will  
come soon.
About inttoptr, actually instcombine tries to fold those instructions  
back to GEPs whenever possible.

Thanks,
Nuno