[llvm-commits] [llvm] r42016 - /llvm/trunk/lib/Analysis/IPA/Andersens.cpp

[Duncan Sands]

Hi,

> Rewrite of andersen's to be about 100x faster, cleaner, and begin to support field sensitivity

some nitpicking comments:

> +// without any issues.  To wit, an indirect call Y(a,b) is equivalence to

equivalence -> equivalent

>  STATISTIC(NumIters            , "Number of iterations to reach convergence");
>  STATISTIC(NumConstraints      , "Number of constraints");
>  STATISTIC(NumNodes            , "Number of nodes");
> -STATISTIC(NumEscapingFunctions, "Number of internal functions that escape");
> -STATISTIC(NumIndirectCallees  , "Number of indirect callees found");
> +STATISTIC(NumUnified          , "Number of variables unified");

there are now a lot of pointless spaces here.  How about aligning commas on the
end of NumConstraints?

> +    /// Constraint - Objects of this structure are used to represent the various
> +    /// constraints identified by the algorithm.  The constraints are 'copy',
> +    /// for statements like "A = B", 'load' for statements like "A = *B",
> +    /// 'store' for statements like "*A = B", and AddressOf for statements like
> +    /// A = alloca;  The Offset is applied as *(A + K) = B for stores,
> +    /// A = *(B + K) for loads, and A = B + K for copies.  It is
> +    /// illegal on addressof constraints (Because it is statically

Because -> because

Also, in this and other comments some lines are rather short, and I can't always see why
you break them early.

> +    struct Node {
> +       Value *Val;
> +      SparseBitVector<> *Edges;

strange indentation of Val.

> +      // compression.  NodeRep gives the index into GraphNodes
> +      // representative for this one.

Probably "the GraphNodes representative"

> +      unsigned NodeRep;    public:

shouldn't "public:" be on the next line?

> +
> +      Node() : Val(0), Edges(0), PointsTo(0), OldPointsTo(0), Changed(false),
> +               NodeRep(SelfRep) {
> +      }

Closing } could follow the opening { on the previous line.

> +    // Map from graph node to maximum K value that is allowed (For functions,

For -> for

> +    // Stack for Tarjans

Tarjan's

> +        // This may look a bit ugly, but what it does is allow us to process
> +        // both store and load constraints with the same function.

with the same function -> within the same function (?)

> +          // Need to increment the member by K since that is where we are
> +          // supposed to copy to/from

Missing full stop.

> +          // Node that in positive weight cycles, which occur in address taking

Node that -> Note that

Ciao,

Duncan.