[PATCH] LLVM CFL Alias Analysis -- Supporting Data Structures
Sanjoy Das
sanjoy at playingwithpointers.com
Tue Jul 29 01:08:44 PDT 2014
Totally bike-shedding comment on terminology: `StratifiedSets.h` uses terms that both leak the fact that we're manipulating memory expressions and aliasing information (`hasAliasingExternal`) and terms that try to hide it (`up`, `down`). It may be clearer to rename either `hasAliasingExternal` to something more general (some property that is transitive in the "low" direction) or `up` / `down` to something more specific (`addressof` / `dereference`).
================
Comment at: lib/Analysis/StratifiedSets.h:135
@@ +134,3 @@
+// %c = call i1 @SomeFunc()
+// %k = select %c, %ap, %bp
+// store %ad, %a
----------------
Should this be `%k = select %c, %ad, %bp`?
================
Comment at: lib/Analysis/StratifiedSets.h:422
@@ +421,3 @@
+ while (current->isRemapped()) {
+ current = &links[current->getRemapIndex()];
+ start->updateRemap(current->number);
----------------
Why only update `start` here? Can't the whole sequence of `SetLink`s remapped to the final index? If it is beneficial to `updateRemap` only `start`, why not do it outside the loop, once we have the first `SetLink` that isn't remapped?
================
Comment at: lib/Analysis/StratifiedSets.h:494
@@ +493,3 @@
+
+ // TODO: Maybe find a better way to do this than a forest of if
+ // statements?
----------------
If I understood the logic correctly here, maybe this can be written not as worklist algorithm, but simply as two loops, each in one direction? So something structurally like this:
```
StratifiedIndex linksIntoIdx = idx1, linksFromIdx = idx2;
while (true) {
auto &linksInto = linksAt(linksIntoIdx);
auto &linksFrom = linksAt(linksFromIdx);
linksInto.remapTo(linksFrom.number);
if (linksInto.hasAbove() && linksFrom.hasAbove()) {
linksIntoIdx = linksInto.getAbove();
linksFromIdx = linksFrom.getAbove();
} else {
break;
}
}
if (linksAt(linksIntoIdx).hasAbove() || linksAt(linksFromIdx).hasAbove()) {
auto &topLink =
linksAt(linksIntoIdx).hasAbove() ?
linksAt(linksIntoIdx).getAbove() : linksAt(linksFromIdx).getAbove();
topLink.setBelow(linksIntoIdx);
linksAt(linksIntoIdx).setAbove(topLink);
}
// ... and a second loop in the below direction
```
If the logic cannot be expressed that way then a comment on why that is the case would help. :)
http://reviews.llvm.org/D4550
More information about the llvm-commits
mailing list