[PATCH] LLVM CFL Alias Analysis -- Supporting Data Structures

[hfinkel at anl.gov]

Hi George,

Thanks for working on this! I apologize ahead of time for not entering these comments on the web site (I was composing this review on the plane). As a general note, you'll need to fix a bunch of compliance issues with LLVM's coding conventions (variable naming, braces, etc.), and while I pointed out a couple of these, I quickly stopped.

+// \brief These are stratified sets, as described in the paper located at
+// www.cse.cuhk.edu.hk/lyu/_media/paper/pldi2013.pdf -- in short, this is meant

This looks like an unofficial link that could become stale. I recommend including the full reference (a search engine can find the PDF, if it is still available, at some future time).

+  class StratifiedInfo;
+
+  class StratifiedInfo {
+  public:

You don't need to pre-declare this one line before defining it. Also, make it a struct.

+// This builder allows the user to add elements with, (at the same level as)

Remove the comma prior to the parenthesis.

+// In the paper above, StratifiedSets are described as a data structure wherein

"In the paper above, StratifiedSets are described as" -> "StratifiedSets are"

+  constexpr static auto SetSentinel =
+      std::numeric_limits<StratifiedIndex>::max();

Add a comment explaining how this is used.

+  // and the set deemed as below it. This also keeps information on the notion
+  // of "remapping",

What does it mean to "keep information on the notion of" somthing? Does this means it manages the state associated with state remapping?

+    // elements that have parents in the union-find structure. This is super
+    // difficult to debug, so we use accessors.

You don't need to justify the use of accessors.

+    void setBelow(StratifiedIndex i) {
+      assert(!isRemapped());
+      below = i;
+    }

Please assert that i != SetSentinel. Same applies to setAbove and remapTo.

+    // aliasedExternal without that check. So we have this.

Remove "So we have this."

+    bool hasAliasedExternalRaw() const { return aliasedExternal; }

It sounds like this should not be public. Make it private and make this a friend of whatever needs access.

+    // \brief For initial remapping to another set
+    void remapTo(StratifiedIndex other) {

What's the reason for using the term "remapping" to describe the set unioning operation? There seem to be better choices (union, join, group; maybe some term related to proxying would be more appropriate?).

+  // Removing dead sets may not be worth it -- "saves" us 20 bits in the average
+  // case (51 sets total) and 4KB in the worst case (out of 1.07M sets == ~133KB
+  // total) when bootstrapping LLVM.

I don't understand this comment in the context of the code. It saves us bits where?

+    while (current->hasAbove()) {
+      current = &linksAt(current->getAbove());
+    }

Remove the {}

+    DenseSet<SetLinks *> visited;

SmallPtrSet<SetLinks, 16> Visisted; (pick some number you feel is reasonable, 16 is just a guess on my part) -- unless you expect the set to be pretty large.

+    bool hasExternals = false;

HasExternals (please follow the LLVM coding conventions for naming variables)

+      if (!insertSuccess) {
+        continue;
+      }

Remove {} (please follow the LLVM coding conventions in this regard)

+  // \brief We store external aliasing information as part of SetLinks, when
+  // it's way more space efficient to store it as a bitset. This generates said
+  // bitset.

This comment implies that this function moves information into a bitset to save space, but it does not. It simply creates the bitset, leaving the original information in the links unchanged.

+    std::vector<bool> aliasingExternals;
+    aliasingExternals.resize(links.size());

Set the size using the constructor.

+    assert(&linksAt(idx1) != &linksAt(idx2));

This assert needs some text because it is not obvious what it is asserting. Make it something like:
  assert(&linksAt(idx1) != &linksAt(idx2) && "Trying to merge two sets with the same <fill in here>");

+    // CASE 1: If `into` is above or below `from`, we need to merge both the

Which one is 'into'?

+      // We want directional searches up/down, but for the initial case, we want
+      // to search both up and down.

Please clarify what 'up/down' means (does this mean 'up or down' or 'up then down').

+    // Funny story: the current implementation of this function makes it

Yea, you know ;)

+      // TODO: Maybe find a better way to do this than a forest of if
+      // statements?

This will look somewhat better once you remove all of the unnecessary {}

+  // TODO: Maybe restructure? I don't like public->private->public

Either do it or remove the comment.

 -Hal

----- Original Message -----
> From: "George Burgess IV" <gbiv at google.com>
> To: gbiv at google.com, chandlerc at gmail.com, nlewycky at google.com, dberlin at dberlin.org
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Friday, July 18, 2014 2:26:40 PM
> Subject: Re: [PATCH] LLVM CFL Alias Analysis -- Supporting Data Structures
> 
> Addressed dberlin's comments, swapped std::map->DenseMap and
> std::set->DenseSet, and took out some unnecessary headers.
> 
> http://reviews.llvm.org/D4550
> 
> Files:
>   lib/Analysis/StratifiedSets.h
>   lib/Analysis/WeightedBidirectedGraph.h
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

http://reviews.llvm.org/D4550