[PATCH] LLVM CFL Alias Analysis -- Algorithm

[Nick Lewycky]

I think this is worth landing and any bug fixes can be incremental development?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:170
@@ +169,3 @@
+  DenseMap<Function *, Optional<FunctionInfo>> Cache;
+  std::forward_list<FunctionHandle> handles;
+
----------------
handles -> Handles, I think>

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:241-242
@@ +240,4 @@
+//    Nothing     - Unknown instruction. Program should die.
+//    Just(false) - Known instruction, but not supported (probably disabled
+//      while trying to find a bug). Discard results from GetEdgesVisitor.
+//    Just(true)  - Known instruction, results are valid. Continue as normal.
----------------
"disabled while trying to find a bug"? An example that explains when this comes up in production use would be better. Unless it really is a debugging feature?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:280
@@ +279,3 @@
+  void visitPHINode(PHINode &Inst) {
+    for (unsigned I = 0, E = Inst.getNumIncomingValues(); I < E; ++I) {
+      Value *Val = Inst.getIncomingValue(I);
----------------
"I < E" is usually "I != E" in llvm, though that causes no particular benefit here.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:286-289
@@ +285,6 @@
+
+  void visitGetElementPtrInst(GetElementPtrInst &Inst) {
+    Value *Op = Inst.getPointerOperand();
+    Output.push_back({&Inst, Op, EdgeWeight::Assign, AttrNone});
+  }
+
----------------
What about the index arguments? Suppose you have 
  inty = ptrtoint ptry
  GEP (ptrx-ptry), inty
?


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:291-296
@@ +290,8 @@
+
+  void visitSelectInst(SelectInst &Inst) {
+    auto *TrueVal = Inst.getTrueValue();
+    auto *FalseVal = Inst.getFalseValue();
+    Output.push_back({&Inst, TrueVal, EdgeWeight::Assign, AttrNone});
+    Output.push_back({&Inst, FalseVal, EdgeWeight::Assign, AttrNone});
+  }
+
----------------
Only the true and false values but not the condition, eh. You wanna hear something horrible?

  void foo(char *p) {
    uintptr_t x = 0;
    uintptr_t y = (uintptr_t)p;
    for (int i = 0; i < 8*sizeof(char*); ++i, x <<= 1) {
      int bit = (y>>(8*sizeof(char*)-1)) & 1;
      x += bit ? 1 : 0;  // SelectInst here.
    }
    char *q = (char*)x;
    // don't answer noalias(p, q).
  }


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:312-314
@@ +311,5 @@
+
+  static bool isFunctionExternal(Function *Fn) {
+    return !Fn->isDeclaration() && Fn->hasLocalLinkage();
+  }
+
----------------
Eh? I would've expected it's external if declaration || !hasLocalLinkage. Is this function the negative of its name?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:386
@@ +385,3 @@
+        StratifiedAttrs Externals;
+        for (unsigned X = 0; X < RetVals.size(); ++X) {
+          auto MaybeInfo = Sets.find(RetVals[X]);
----------------
Does RetVals change size inside this loop? If not, the "for (unsigned X = 0, XE = RetVals.size(); X != XE; ++X) {" formulation makes that clear.

(Oh and again at line 381 for Parameters, 415 for Arguments.)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:406-407
@@ +405,4 @@
+
+      if (Parameters.size() != Arguments.size())
+        return false;
+
----------------
You could also check Fn->isVarArgs() way up in the early exit detection code instead? If it's not varargs then #params = #args? Unless the call is indirect (ie., through a bitcast).

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:414
@@ +413,3 @@
+      // Arguments[I] and Arguments[X], but our algorithm will produce
+      // extermely similar, and equally correct, results either way)
+      for (unsigned I = 0; I < Arguments.size(); ++I) {
----------------
Typo, "extermely" -> "extremely".

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:738-739
@@ +737,4 @@
+    while (!Worklist.empty()) {
+      auto Node = Worklist.back();
+      Worklist.pop_back();
+
----------------
auto Node = Worklist.pop_back_val()?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:793
@@ +792,3 @@
+//===----------------------------------------------------------------------===//
+// Lengthy CFL AA methods
+//===----------------------------------------------------------------------===//
----------------
"Lengthy"?

http://reviews.llvm.org/D4551