[PATCH] LLVM CFL Alias Analysis -- Algorithm

[Hal Finkel]

----- Original Message -----
> From: "Nick Lewycky" <nlewycky at google.com>
> To: "George Burgess" <gbiv at google.com>, "Chandler Carruth" <chandlerc at gmail.com>, "Daniel Berlin"
> <dberlin at dberlin.org>, "Hal Finkel" <hfinkel at anl.gov>, "Nicholas Lewycky" <nlewycky at google.com>
> Cc: liujiangning1 at gmail.com, "george burgess iv" <george.burgess.iv at gmail.com>, sanjoy at playingwithpointers.com,
> "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Wednesday, August 20, 2014 8:09:41 PM
> Subject: Re: [PATCH] LLVM CFL Alias Analysis -- Algorithm
> 
> 
> 
> 
> 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?

Please do. I'll review these again shortly.

 -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
> 
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory