[llvm-commits] [PATCH] New pass: pointer (bounds) tracking!

[Török Edwin]

On 2009-07-14 20:56, Dan Gohman wrote:
> On Jul 13, 2009, at 1:59 PM, Török Edwin wrote:
>   
>> I fixed this in pointertracking_notready.patch by checking that:
>> - there is one and only one successor  of the branch that dominates  
>> the
>> target BB, excluding backedges
>> -  all predecessors of the targetBB are dominated by that successor
>>     
>
> There are still cases that this doesn't catch. For example in this CFG:
>
>    BranchBB
>    /     \
> OtherBB  |
>     \     |
>      \    |
>        DomBB
>          |
>        TargetBB
>
> BranchBB has exactly one successor (DomBB) which dominates TargetBB,
> and all of TargetBB's predecessors (it only has one, DomBB) are
> dominated by that successor. However, TargetBB is still reachable
> from OtherBB.
>   

Right, I think I can't avoid traversing the inverse CFG.
I'll think more about this.

> I think pointertracking1.patch can be checked in when you're ready;
> you can continue to work on it after it's in the tree. 

Ok, I'll check it in shortly. Thanks for the review!

> Here are a
> few comments on the pointertracking1.patch for now:
>
>  > +
>  > +#include "llvm/Analysis/LoopInfo.h"
>
> FWIW, it's now possible to forward-declare Loop and LoopInfo, so you
> can move this #include "llvm/Analysis/LoopInfo.h" to  
> PointerTracking.cpp.
>   

Ok.


>  > +  if (GlobalVariable *GV = dyn_cast<GlobalVariable>(V)) {
>  > +    if (GV->hasInitializer()) {
>  > +      // FIXME: should check for weak/non-overriddable symbol here?
>
> GV->mayBeOverridden() is the check.
>   

Ok, I used hasDefinitiveInitializer which combines hasInitializer with
!mayBeOverridden.

>  > +
>  > +    unsigned want_bits = Ty->getPrimitiveSizeInBits();
>  > +    unsigned have_bits = elementTy->getPrimitiveSizeInBits();
>  > +    if (have_bits && want_bits) {
>  > +      if (want_bits == have_bits)
>  > +        return Count;
>  > +      if (have_bits % want_bits) //fractional counts not possible
>  > +          return SE->getCouldNotCompute();
>  > +      return SE->getMulExpr(Count, SE->getConstant(Count->getType(),
>  > +                                                   have_bits/ 
> want_bits));
>  > +    }
>
> This doesn't handle odd-sized integer types like i1 and i2 correctly.  
> For
> these, getPrimitiveSizeInBits returns 1 and 2, respectively, but both
> are typically allocated in memory in one byte. I appreciate that you're
> trying to support the no-TargetData case as much as possible, but I  
> think
> the only thing that can be done is the elementTy == Ty case above.
>   

Ok.
Maybe the comment on getPrimitiveSizeInBits should be updated to reflect
that the allocation/store size *is* affected by TargetData?


> Also, FWIW, LLVM style tends to use more capital letters and fewer
> underscores.
>   

I'll keep that in mind.

>  > +
>  > +    if (!TD) // need TargetData from this point forward
>  > +      return SE->getCouldNotCompute();
>  > +
>  > +    uint64_t elementsize = TD->getTypeAllocSize(elementTy);
>  > +    uint64_t wantsize = TD->getTypeStoreSize(Ty);
>  > +    if (elementsize == wantsize)
>  > +      return Count;
>  > +    if (elementsize % wantsize) //fractional counts not possible
>  > +      return SE->getCouldNotCompute();
>  > +    return SE->getMulExpr(Count, SE->getConstant(Count->getType(),
>  > +                                                 elementsize/ 
> wantsize));
>  > +}
>
> Since you're doing elementsize/wantsize etc., both sizes need to be the
> AllocSize here, not the StoreSize.
>   

Ok.

Best regards,
--Edwin