[PATCH] LLVM CFL Alias Analysis -- Algorithm

[hfinkel at anl.gov]

Hi again,

+// algorithm. It does not depend on types The algorithm is a mixture of the one
+// described here: www.cs.cornell.edu/~rugina/papers/popl08.pdf and here:
+// www.cse.cuhk.edu.hk/lyu/_media/paper/pldi2013.pdf -- to summarize the papers,

Again, actual references please.

+static bool getPossibleTargets(Inst *, SmallVectorImpl<Function *> *);

Why is this not SmallVectorImpl<Function *> & ?

+// There are certain instructions (i.e. FenceInst, etc.) that we ignore.
+// This detects those.
+static bool hasUsefulEdges(Instruction *inst);

This returns true when we don't ignore the instruction?

+  // TODO: Maybe merge this with AssignFrom if it carries no benefit.

Please place this comment near the relevant code.

+  // TODO: Rename this + Reference

Okay, please do.

+  // %b = load %a              (%b Dereference %a)
+  // %b = extractelement %a, 0 (%b Dereference %a)

I suppose this second item is what motivates the renaming?

+  /// If a function's sets arecurrently being built, it is marked

are currently

+struct CFLAliasAnalysis : public ImmutablePass, public AliasAnalysis {
+private:
...
+  DenseMap<Function *, Optional<StratifiedSets<Value *>>> cache;

This is an ImmutablePass and keeps a cache of Function* -- This is dangerous because a function would be deleted and new one created at the same address. You probably want to use a CallbackVH from llvm/IR/ValueHandle.h so that you can get notified if the Fuction is removed and remove it from the cache. (The ScalarEvolution analysis does something like this).

+    // This should be covered by BasicAA, but it's super cheap to check.

As a genral note, you must not constuct an AA pass assuming that BasicAA will filter for you. Your pass must independently return correct answers for all cases.

+// Currently everything returns an Optional<bool> because it makes life easier
+// to be able to selectively disable support for instructions and troubleshoot
+// our coverage of the instructions we'll get. I plan to remove this as this AA
+// gets tested more/nearer finalization.

Alright, please add a FIXME, an assert, an llvm_unreachable, or something.

+//    Nothing     - Unknown instruction. Program should die.

Let's leave dying for another day.

(Actually, if you're ready to remove the Optional now, please do).

+  CFLAliasAnalysis *aa;
+  SmallVectorImpl<Edge> *output;

These can be refeences?

+  Optional<bool> visitInstruction(Instruction &) { return NoneType(); }

llvm_unreachable("CFL AA does not know about this kind of instruction");

+  Optional<bool> visitBinaryOperator(BinaryOperator &inst) {
+    auto *op1 = inst.getOperand(0);
+    auto *op2 = inst.getOperand(1);
+    output->push_back({&inst, op1, EdgeWeight::AssignFrom, false});
+    output->push_back({&inst, op2, EdgeWeight::AssignFrom, false});

Do we actually need edges for all binary operators, or only those that could be part of pointer arithmetic? Meaning, can we exclude the floating-point operators, for example?

+    for (unsigned i = 0, e = inst.getNumIncomingValues(); i < e; ++i) {
+      Value *val = inst.getIncomingValue(i);
+      output->push_back({&inst, val, EdgeWeight::AssignFrom, false});
+    }

Does it matter if you add duplicate edges here? A funny property of LLVM phis is that they can have duplicate entries for the same predecessor block so long as the values also match.

+  static bool isFunctionExternal(Function *fn) {
+    return fn->getBasicBlockList().empty();
+  }

This is not the correct way to test for this. I think you want !fn->isDeclaration() && fn->hasLocalLinkage().

+    bool onlyReadMemory =
+        std::all_of(fns.begin(), fns.end(),
+                    [](const Function *fn) { return fn->onlyReadsMemory(); });

What does this check have to do with IPA? If this property is true, then it is true regardless of whether the function is local or not. This is how it is defined (it is definitional, not speculative).

+    for (Value *v : inst.arg_operands()) {
+      output->push_back({&inst, v, EdgeWeight::AssignFrom, true});
+    }

Do you need this for calls returning void?

+  Optional<bool> visitShuffleVectorInst(ShuffleVectorInst &inst) {
+    auto *from1 = inst.getOperand(0);
+    auto *from2 = inst.getOperand(1);
+    output->push_back({&inst, from1, EdgeWeight::AssignFrom, false});
+    output->push_back({&inst, from2, EdgeWeight::AssignFrom, false});
+    return true;
+  }

Many vector shuffles have an Undef second operand, I assume you don't need an edge for those. On that thought, you should probably filter out edges from undef everywhere.

+class GetTargetValueVisitor
+    : public InstVisitor<GetTargetValueVisitor, Optional<Value *>> {
+public:
+  Optional<Value *> visitInstruction(Instruction &) { return NoneType(); }
+  Optional<Value *> visitCastInst(CastInst &inst) { return &inst; }
+  Optional<Value *> visitBinaryOperator(BinaryOperator &inst) { return &inst; }
+  Optional<Value *> visitLoadInst(LoadInst &inst) { return &inst; }
+  Optional<Value *> visitSelectInst(SelectInst &inst) { return &inst; }
+  Optional<Value *> visitAllocaInst(AllocaInst &inst) { return &inst; }
+  Optional<Value *> visitPHINode(PHINode &inst) { return &inst; }
+  Optional<Value *> visitCallInst(CallInst &inst) { return &inst; }
+  Optional<Value *> visitInvokeInst(InvokeInst &inst) { return &inst; }
+  Optional<Value *> visitShuffleVectorInst(ShuffleVectorInst &inst) {
+    return &inst;
+  }
...

Just provide a default using visitInstruction only override the special cases.

+// edges. True if we could, false if we couldn't. If you hand this function an
+// instruction it didn't expect, we break. Yay. (For more information on what it
+// expects/how to add instruction support, see GetEdgesVisitor)

Make sure you handle all instructions, obviously, then remove the latter part of this comment.

+//===----------------------------------------------------------------------===//
+// Definitions of static functions declared at the top/middle of the file
+//===----------------------------------------------------------------------===//

This comment is unnecessary.

+// TODO: Make this look at virtual calls, etc.

Exactly what do you have in mind?

+  bool isExcitingTerminator =
+      isa<InvokeInst>(inst) || isa<LandingPadInst>(inst);

Exciting! Just fold this into the return conditional.

+static bool isAliasingExternal(Value *val) {
+  bool isGlobal = isa<GlobalValue>(val);
+  auto *arg = dyn_cast<Argument>(val);
+  bool isAliasArg = arg != nullptr && !arg->hasNoAliasAttr();
+  return isGlobal || isAliasArg;
+}

You're missing handling for byval arguments. Look at isIdentifiedObject and friends in AliasAnalysis.cpp

+// Aside: We may remove graph construction entirely, because it doesn't really
+// buy us much that we don't already have. I'd like to add interprocedural

Can you please be more specific about what it does or does not buy us?

+  if (!maybeFnA.hasValue() && !maybeFnB.hasValue()) {
+    llvm_unreachable("Can't find Funcation A or Function B");
+  }

Funcation -> Function; also be more specific, how about "Don't know how to extract the parent function from this value".

+    // Ensure we're looking at the same function for both variables.
+    assert(!maybeFnB.hasValue() || *maybeFnB == *maybeFnA);

() && "Interprocedural queries not supported"

+  if (!maybeA.hasValue()) {
+    return NoneType();
+  }
+
+  auto maybeB = sets.find(valB);
+  if (!maybeB.hasValue()) {
+    return NoneType();
+  }

I see no reason to use Optional here. Just return MayAlias.

Thanks again,
Hal

----- Original Message -----
> From: "George Burgess IV" <gbiv at google.com>
> To: gbiv at google.com, nlewycky at google.com, chandlerc at gmail.com, dberlin at dberlin.org, hfinkel at anl.gov
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Friday, July 25, 2014 3:14:14 PM
> Subject: Re: [PATCH] LLVM CFL Alias Analysis -- Algorithm
> 
> +hfinkel
> 
> Addressed Danny's comments, fixed a bug where we wouldn't properly
> handle exceptions, (i.e. in try {} catch (Foo& f) {}, f would be
> ignored, when it really needs to be treated like an aliasing
> external.) and added an extra field to Edge so interprocedural is
> easier to integrate.
> 
> http://reviews.llvm.org/D4551
> 
> Files:
>   include/llvm/Analysis/Passes.h
>   include/llvm/InitializePasses.h
>   include/llvm/LinkAllPasses.h
>   lib/Analysis/CFLAliasAnalysis.cpp
>   lib/Analysis/CMakeLists.txt
>

http://reviews.llvm.org/D4551