[PATCH] LLVM CFL Alias Analysis -- Supporting Data Structures
hfinkel at anl.gov
hfinkel at anl.gov
Sat Jul 26 17:55:31 PDT 2014
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
More information about the llvm-commits
mailing list