[llvm-commits] [llvm] r84038 - in /llvm/trunk: lib/Analysis/BasicAliasAnalysis.cpp test/Analysis/BasicAA/phi-aa.ll

[Evan Cheng]

On Oct 13, 2009, at 3:31 PM, Chris Lattner wrote:

> 
> On Oct 13, 2009, at 3:02 PM, Evan Cheng wrote:
> 
>> Author: evancheng
>> Date: Tue Oct 13 17:02:20 2009
>> New Revision: 84038
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=84038&view=rev
>> Log:
>> Teach basic AA about PHI nodes. If all operands of a phi NoAlias another value than it's safe to declare the PHI NoAlias the value. Ditto for MustAlias.
> 
> Thoughts:
> 
> Please manually reduce the testcase (or better yet, synthesize one from scratch).  The basicaa tests are almost all very small and precise, e.g. 2007-08-01-NoAliasAndCalls.ll.  Also, please don't add new uses of the malloc instruction, use alloca instead.  Something like this (in hand written IR) should be enough:
> 
> int X;
> int Y;
> int Z;
> void foo(int cond) {
>  int *P = cond ? &X : &Y;
> 
>  int tmp = Z;
>  *P = 123;
>  return tmp-Z;
> }

I'll try.

> 
> 
> Did you look at the compile time impact of this on something like GVN on 403.gcc?

I am not seeing any compile time impact.

> 
> Because you only have one 'VisitedPHIs' set, you are conflating PHIs walked on the LHS with phis walked on the RHS.  I guess it probably doesn't matter because if the same phi shows up on both sides, they definitely alias.  This is worth mentioning in a comment somewhere.

The analysis stops asap it hits a phi source which is itself a phi. So the worst possible case if both LHS and RHS are phis. In that case, its m x n.

> 
>> @@ -218,7 +222,16 @@
>>    // aliasGEP - Provide a bunch of ad-hoc rules to disambiguate a GEP instruction
>>    // against another.
>>    AliasResult aliasGEP(const Value *V1, unsigned V1Size,
>> -                         const Value *V2, unsigned V2Size);
>> +                         const Value *V2, unsigned V2Size,
>> +                         SmallSet<const Value*, 16> &VisitedPHIs);
>> +
>> +    AliasResult aliasPHI(const Value *V1, unsigned V1Size,
>> +                         const Value *V2, unsigned V2Size,
>> +                         SmallSet<const Value*, 16> &VisitedPHIs);
>> +
>> +    AliasResult aliasCheck(const Value *V1, unsigned V1Size,
>> +                           const Value *V2, unsigned V2Size,
>> +                           SmallSet<const Value*, 16> &VisitedPHIs);
> 
> Somewhat gross, but I think it would be better to make VisitedPHIs be an instance variable in the BasicAA class.  Just assert that it is empty on entry into and exit out of the top level "alias" method.

Alright.

> 
> 
>> +AliasAnalysis::AliasResult
>> +BasicAliasAnalysis::aliasPHI(const Value *V1, unsigned V1Size,
> 
> V1 is statically required to be a PHI, it would be better for it to be declared as 'const PHINode*'.  Also, the VisitedPHI set should be PHINode not Value as element type.
> 
>> +  SmallSet<Value*, 4> UniqueSrc;
>> +  SmallVector<Value*, 4> V1Srcs;
>> +  const PHINode *PN = cast<PHINode>(V1);
> 
> this cast<> should be removed.
> 
>> +  // If all sources of the PHI node NoAlias or MustAlias V2, then returns
>> +  // NoAlias / MustAlias. Otherwise, returns MayAlias.
>> +  AliasResult Alias = aliasCheck(V1Srcs[0], V1Size, V2, V2Size, VisitedPHIs);
> 
> if this call returns mayalias, you should exit early.  mayalias is the most common result.

Ok.

Evan

> 
> -Chris
>