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

[Chris Lattner]

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;
}


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

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.

> @@ -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.


> +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.

-Chris