CFLAA -- ConstantExpr Awareness

[george.burgess.iv]

Summary

// Hal already LGTM'ed an earlier version of this patch -- there are a few minor revisions that have been made to it.

Makes CFLAA aware of ConstantExpr Value*s, and how to traverse into them to get to properly handle their non-ConstantExpr parts.

Accuracy drop in tests is part of a minor bug fix where we wouldn't properly tag arguments with StratifiedAttrs if said arguments were never otherwise put in the Stratified Sets.

[majnemer]

lib/Analysis/CFLAliasAnalysis.cpp

966–976

What if the constant is a ConstantArray/ConstantStruct/ConstantVector with a GlobalValue field?

[george.burgess.iv]

Addressed David's feedback, fixed a bug where we'd hit llvm_unreachable upon attempting to bootstrap LLVM.

[• dberlin]

LGTM algorithmically

[majnemer]

LGTM

[george.burgess.iv]

r231743.

lib/Analysis/CFLAliasAnalysis.cpp

 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 #include <cassert>
+#include <memory>
 #include <forward_list>
 #include <tuple>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "cfl-aa"
 
 // Try to go from a Value* to a Function*. Never returns nullptr.
if (LocA.Ptr == LocB.Ptr) {
         return MustAlias;
       } else {
         return PartialAlias;
       }
     }
 
     // Comparisons between global variables and other constants should be
     // handled by BasicAA.
+    // TODO: ConstantExpr handling -- CFLAA may report NoAlias when comparing
+    // a GlobalValue and ConstantExpr, but every query needs to have at least
+    // one Value tied to a Function, and neither GlobalValues nor ConstantExprs
+    // are.
     if (isa<Constant>(LocA.Ptr) && isa<Constant>(LocB.Ptr)) {
       return AliasAnalysis::alias(LocA, LocB);
     }
 
     AliasResult QueryResult = query(LocA, LocB);
     if (QueryResult == MayAlias)
       return AliasAnalysis::alias(LocA, LocB);
 
tryInterproceduralAnalysis(const SmallVectorImpl<Function *> &Fns,
                              Value *FuncValue,
                              const iterator_range<User::op_iterator> &Args) {
     const unsigned ExpectedMaxArgs = 8;
     const unsigned MaxSupportedArgs = 50;
     assert(Fns.size() > 0);
 
     // I put this here to give us an upper bound on time taken by IPA. Is it
     // really (realistically) needed? Keep in mind that we do have an n^2 algo.
-    if (std::distance(Args.begin(), Args.end()) > (int) MaxSupportedArgs)
+    if (std::distance(Args.begin(), Args.end()) > (int)MaxSupportedArgs)
       return false;
 
     // Exit early if we'll fail anyway
     for (auto *Fn : Fns) {
       if (isFunctionExternal(Fn) || Fn->isVarArg())
         return false;
       auto &MaybeInfo = AA.ensureCached(Fn);
       if (!MaybeInfo.hasValue())

 // given an EdgeType.
 static Level directionOfEdgeType(EdgeType);
 
 // Builds the graph needed for constructing the StratifiedSets for the
 // given function
 static void buildGraphFrom(CFLAliasAnalysis &, Function *,
                            SmallVectorImpl<Value *> &, NodeMapT &, GraphT &);
 
+// Gets the edges of a ConstantExpr as if it was an Instruction. This
+// function also acts on any nested ConstantExprs, adding the edges
+// of those to the given SmallVector as well.
+static void constexprToEdges(CFLAliasAnalysis &, ConstantExpr &,
+                             SmallVectorImpl<Edge> &);
+
+// Given an Instruction, this will add it to the graph, along with any
+// Instructions that are potentially only available from said Instruction
+// For example, given the following line:
+//   %0 = load i16* getelementptr ([1 x i16]* @a, 0, 0), align 2
+// addInstructionToGraph would add both the `load` and `getelementptr`
+// instructions to the graph appropriately.
+static void addInstructionToGraph(CFLAliasAnalysis &, Instruction &,
+                                  SmallVectorImpl<Value *> &, NodeMapT &,
+                                  GraphT &);
+
+// Notes whether it would be pointless to add the given Value to our sets.
+static bool canSkipAddingToSets(Value *Val);
+
 // Builds the graph + StratifiedSets for a function.
 static FunctionInfo buildSetsFrom(CFLAliasAnalysis &, Function *);
 
 static Optional<Function *> parentFunctionOfValue(Value *Val) {
   if (auto *Inst = dyn_cast<Instruction>(Val)) {
     auto *Bb = Inst->getParent();
     return Bb->getParent();
   }
static EdgeType flipWeight(EdgeType Initial) {
   case EdgeType::Reference:
     return EdgeType::Dereference;
   }
   llvm_unreachable("Incomplete coverage of EdgeType enum");
 }
 
 static void argsToEdges(CFLAliasAnalysis &Analysis, Instruction *Inst,
                         SmallVectorImpl<Edge> &Output) {
+  assert(hasUsefulEdges(Inst) &&
+         "Expected instructions to have 'useful' edges");
   GetEdgesVisitor v(Analysis, Output);
   v.visit(Inst);
 }
 
 static Level directionOfEdgeType(EdgeType Weight) {
   switch (Weight) {
   case EdgeType::Reference:
     return Level::Above;
   case EdgeType::Dereference:
     return Level::Below;
   case EdgeType::Assign:
     return Level::Same;
   }
   llvm_unreachable("Incomplete switch coverage");
 }
 
-// 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
-// analysis prior to this however, in case that somehow requires the graph
-// produced by this for efficient execution
-static void buildGraphFrom(CFLAliasAnalysis &Analysis, Function *Fn,
+static void constexprToEdges(CFLAliasAnalysis &Analysis,
+                             ConstantExpr &CExprToCollapse,
+                             SmallVectorImpl<Edge> &Results) {
+  SmallVector<ConstantExpr *, 4> Worklist;
+  Worklist.push_back(&CExprToCollapse);
+
+  SmallVector<Edge, 8> ConstexprEdges;
+  while (!Worklist.empty()) {
+    auto *CExpr = Worklist.pop_back_val();
+    std::unique_ptr<Instruction> Inst(CExpr->getAsInstruction());
+
+    if (!hasUsefulEdges(Inst.get()))
+      continue;
+
+    ConstexprEdges.clear();
+    argsToEdges(Analysis, Inst.get(), ConstexprEdges);
+    for (auto &Edge : ConstexprEdges) {
+      if (Edge.From == Inst.get())
+        Edge.From = CExpr;
+      else if (auto *Nested = dyn_cast<ConstantExpr>(Edge.From))
+        Worklist.push_back(Nested);
+
+      if (Edge.To == Inst.get())
+        Edge.To = CExpr;
+      else if (auto *Nested = dyn_cast<ConstantExpr>(Edge.To))
+        Worklist.push_back(Nested);
+    }
+
+    Results.append(ConstexprEdges.begin(), ConstexprEdges.end());
+  }
+}
+
+static void addInstructionToGraph(CFLAliasAnalysis &Analysis, Instruction &Inst,
                                   SmallVectorImpl<Value *> &ReturnedValues,
                                   NodeMapT &Map, GraphT &Graph) {
   const auto findOrInsertNode = [&Map, &Graph](Value *Val) {
     auto Pair = Map.insert(std::make_pair(Val, GraphT::Node()));
     auto &Iter = Pair.first;
     if (Pair.second) {
       auto NewNode = Graph.addNode();
       Iter->second = NewNode;
     }
     return Iter->second;
   };
 
-  SmallVector<Edge, 8> Edges;
-  for (auto &Bb : Fn->getBasicBlockList()) {
-    for (auto &Inst : Bb.getInstList()) {
-      // We don't want the edges of most "return" instructions, but we *do* want
-      // to know what can be returned.
-      if (auto *Ret = dyn_cast<ReturnInst>(&Inst))
-        ReturnedValues.push_back(Ret);
+  if (isa<ReturnInst>(&Inst))
+    ReturnedValues.push_back(&Inst);
 
-      if (!hasUsefulEdges(&Inst))
-        continue;
+    return;
 
-      Edges.clear();
-      argsToEdges(Analysis, &Inst, Edges);
+  SmallVector<Edge, 8> Edges;
 
-      // In the case of an unused alloca (or similar), edges may be empty. Note
-      // that it exists so we can potentially answer NoAlias.
-      if (Edges.empty()) {
-        auto MaybeVal = getTargetValue(&Inst);
-        assert(MaybeVal.hasValue());
-        auto *Target = *MaybeVal;
-        findOrInsertNode(Target);
-        continue;
-      }
+    return;
 
-      for (const Edge &E : Edges) {
-        auto To = findOrInsertNode(E.To);
-        auto From = findOrInsertNode(E.From);
-        auto FlippedWeight = flipWeight(E.Weight);
-        auto Attrs = E.AdditionalAttrs;
-        Graph.addEdge(From, To, std::make_pair(E.Weight, Attrs),
-                                std::make_pair(FlippedWeight, Attrs));
-      }
-    }
-  }
+  const auto addEdgeToGraph = [&Graph, &findOrInsertNode](const Edge &E) {
+  };
+
+  SmallVector<ConstantExpr *, 4> ConstantExprs;
+  for (const Edge &E : Edges) {
+    addEdgeToGraph(E);
+    if (auto *Constexpr = dyn_cast<ConstantExpr>(E.To))
+      ConstantExprs.push_back(Constexpr);
+    if (auto *Constexpr = dyn_cast<ConstantExpr>(E.From))
+      ConstantExprs.push_back(Constexpr);
+
+  for (ConstantExpr *CE : ConstantExprs) {
+    Edges.clear();
+    constexprToEdges(Analysis, *CE, Edges);
+    std::for_each(Edges.begin(), Edges.end(), addEdgeToGraph);
+
+// 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
+// analysis prior to this however, in case that somehow requires the graph
+// produced by this for efficient execution
+static void buildGraphFrom(CFLAliasAnalysis &Analysis, Function *Fn,
+                           SmallVectorImpl<Value *> &ReturnedValues,
+                           NodeMapT &Map, GraphT &Graph) {
+  for (auto &Bb : Fn->getBasicBlockList())
+    for (auto &Inst : Bb.getInstList())
+      addInstructionToGraph(Analysis, Inst, ReturnedValues, Map, Graph);
+}
+
+static bool canSkipAddingToSets(Value *Val) {
+  // Constants can share instances, which may falsely unify multiple
+  // sets, e.g. in
+  // store i32* null, i32** %ptr1
+  // store i32* null, i32** %ptr2
+  // clearly ptr1 and ptr2 should not be unified into the same set, so
+  // we should filter out the (potentially shared) instance to
+  // i32* null.
+  if (isa<Constant>(Val)) {
+    bool Container = isa<ConstantVector>(Val) || isa<ConstantArray>(Val) ||
+                     isa<ConstantStruct>(Val);
+    // TODO: Because all of these things are constant, we can determine whether
+    // the data is *actually* mutable at graph building time. This will probably
+    // come for free/cheap with offset awareness.
+    bool CanStoreMutableData =
+        isa<GlobalValue>(Val) || isa<ConstantExpr>(Val) || Container;
+    return !CanStoreMutableData;
+  }
+
+  return false;
 }
 
 static FunctionInfo buildSetsFrom(CFLAliasAnalysis &Analysis, Function *Fn) {
   NodeMapT Map;
   GraphT Graph;
   SmallVector<Value *, 4> ReturnedValues;
 
   buildGraphFrom(Analysis, Fn, ReturnedValues, Map, Graph);
for (auto &Pair : Map) {
 
     auto *Value = Pair.first;
     Builder.add(Value);
     auto InitialNode = Pair.second;
     Worklist.push_back(InitialNode);
     while (!Worklist.empty()) {
       auto Node = Worklist.pop_back_val();
       auto *CurValue = findValueOrDie(Node);
-      if (isa<Constant>(CurValue) && !isa<GlobalValue>(CurValue))
+      if (canSkipAddingToSets(CurValue))
         continue;
 
       for (const auto &EdgeTuple : Graph.edgesFor(Node)) {
         auto Weight = std::get<0>(EdgeTuple);
         auto Label = Weight.first;
         auto &OtherNode = std::get<1>(EdgeTuple);
         auto *OtherValue = findValueOrDie(OtherNode);
 
-        if (isa<Constant>(OtherValue) && !isa<GlobalValue>(OtherValue))
+        if (canSkipAddingToSets(OtherValue))
           continue;
 
         bool Added;
         switch (directionOfEdgeType(Label)) {
         case Level::Above:
           Added = Builder.addAbove(CurValue, OtherValue);
           break;
         case Level::Below:
for (auto &Pair : Map) {
     }
   }
 
   // There are times when we end up with parameters not in our graph (i.e. if
   // it's only used as the condition of a branch). Other bits of code depend on
   // things that were present during construction being present in the graph.
   // So, we add all present arguments here.
   for (auto &Arg : Fn->args()) {
-    Builder.add(&Arg);
+    if (!Builder.add(&Arg))
+      continue;
+
+    auto Attrs = valueToAttrIndex(&Arg);
+    if (Attrs.hasValue())
+      Builder.noteAttributes(&Arg, *Attrs);
   }
 
   return FunctionInfo(Builder.build(), std::move(ReturnedValues));
 }
 
 void CFLAliasAnalysis::scan(Function *Fn) {
   auto InsertPair = Cache.insert(std::make_pair(Fn, Optional<FunctionInfo>()));
   (void)InsertPair;

test/Analysis/CFLAliasAnalysis/asm-global-bugfix.ll

 ; Test case for a bug where we would crash when we were requested to report
 ; whether two values that didn't belong to a function (i.e. two globals, etc)
 ; aliased.
 
 ; RUN: opt < %s -cfl-aa -aa-eval -print-may-aliases -disable-output 2>&1 | FileCheck %s
 
 @G = private unnamed_addr constant [1 x i8] c"\00", align 1
 
 ; CHECK: Function: test_no_crash
-; CHECK: 1 no alias responses
+; CHECK: 0 no alias responses
 define void @test_no_crash() #0 {
 entry:
   call i8* asm "nop", "=r,r"(
        i8* getelementptr inbounds ([1 x i8]* @G, i64 0, i64 0))
   ret void
 }

test/Analysis/CFLAliasAnalysis/const-expr-gep.ll

 ; This testcase consists of alias relations which should be completely
 ; resolvable by cfl-aa, but require analysis of getelementptr constant exprs.
 ; Derived from BasicAA/2003-12-11-ConstExprGEP.ll
 
 ; RUN: opt < %s -cfl-aa -aa-eval -print-may-aliases -disable-output 2>&1 | FileCheck %s
 
 %T = type { i32, [10 x i8] }
 
 @G = external global %T
+@G2 = external global %T
 
-; CHECK:     Function: test
-; CHECK-NOT:   May:
+; TODO: Quite a few of these are MayAlias because we don't yet consider
+; constant offsets in CFLAA. If we start doing so, then we'll need to
+; change these test cases
 
+; CHECK:     Function: test
+; CHECK:     MayAlias: i32* %D, i32* %F
+; CHECK:     MayAlias: i32* %D, i8* %X
+; CHECK:     MayAlias: i32* %F, i8* %X
 define void @test() {
   %D = getelementptr %T, %T* @G, i64 0, i32 0
-  %E = getelementptr %T, %T* @G, i64 0, i32 1, i64 5
   %F = getelementptr i32, i32* getelementptr (%T* @G, i64 0, i32 0), i64 0
   %X = getelementptr [10 x i8], [10 x i8]* getelementptr (%T* @G, i64 0, i32 1), i64 0, i64 5
 
   ret void
 }
+
+; CHECK:     Function: simplecheck
+; CHECK:     MayAlias: i32* %F, i32* %arg0
+; CHECK:     MayAlias: i32* %H, i32* %arg0
+; CHECK:     MayAlias: i32* %F, i32* %H
+define void @simplecheck(i32* %arg0) {
+  %F = getelementptr i32, i32* getelementptr (%T* @G, i64 0, i32 0), i64 0
+  %H = getelementptr %T, %T* @G2, i64 0, i32 0
+
+  ret void
+}
+
+; Ensure that CFLAA properly identifies and handles escaping variables (i.e.
+; globals) in nested ConstantExprs
+
+; CHECK:      Function: checkNesting
+; CHECK:      MayAlias: i32* %A, i32* %arg0
+
+%NestedT = type { [1 x [1 x i32]] }
+@NT = external global %NestedT
+
+define void @checkNesting(i32* %arg0) {
+  %A = getelementptr [1 x i32],
+         [1 x i32]* getelementptr
+           ([1 x [1 x i32]]* getelementptr (%NestedT* @NT, i64 0, i32 0),
+           i64 0,
+           i32 0),
+         i64 0,
+         i32 0
+  ret void
+}

test/Analysis/CFLAliasAnalysis/stratified-attrs-indexing.ll

define void @test(i1 %cond,
                   i32* %arg6, i32* %arg7, i32* %arg8, i32* %arg9, i32* %arg10,
                   i32* %arg11, i32* %arg12, i32* %arg13, i32* %arg14, i32* %arg15,
                   i32* %arg16, i32* %arg17, i32* %arg18, i32* %arg19, i32* %arg20,
                   i32* %arg21, i32* %arg22, i32* %arg23, i32* %arg24, i32* %arg25,
                   i32* %arg26, i32* %arg27, i32* %arg28, i32* %arg29, i32* %arg30,
                   i32* %arg31, i32* %arg32, i32* %arg33, i32* %arg34, i32* %arg35) {
 
   ; CHECK: 946 Total Alias Queries Performed
-  ; CHECK: 810 no alias responses (85.6%)
+  ; CHECK: 43 no alias responses (4.5%)
   %a = alloca i32, align 4
   %b = select i1 %cond, i32* %arg35, i32* %arg34
   %c = select i1 %cond, i32* %arg34, i32* %arg33
   %d = select i1 %cond, i32* %arg33, i32* %arg32
   %e = select i1 %cond, i32* %arg32, i32* %arg31
   %f = select i1 %cond, i32* %arg31, i32* %arg30
   %g = select i1 %cond, i32* %arg30, i32* %arg29
   %h = select i1 %cond, i32* %arg29, i32* %arg28
   %i = select i1 %cond, i32* %arg28, i32* %arg27
 
   ret void
 }