[PATCH] LLVM CFL Alias Analysis -- Algorithm

[Nick Lewycky]

On 20 August 2014 18:08, Nick Lewycky <nlewycky at google.com> wrote:

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

...but wait for Hal?

Nick

================
> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140820/d711543b/attachment.html>