[CFLAA] Change FunctionHandle to be common to Steensgaard's and Andersens'

[davide]

Summary

In order to fix https://bugs.llvm.org/show_bug.cgi?id=33339 we need to keep CallBackVH for instructions inserted in the stratified sets so that we can remove when, e.g. instcombine or any other transform remove the instruction.

When looking at fixing this I noticed that Andersens' and Steensgaard's analyses end up duplicating a considerable amount of code, including the cache. This is a first step towards the unification of what's common to the two analyses.

[george.burgess.iv]

LGTM with two nits. Thanks!

include/llvm/Analysis/CFLAliasAnalysisUtils.h

10 ↗

(On Diff #104005)

Nit: Would "These are the utilities/helpers [...]" be better here?

lib/Analysis/CFLAndersAliasAnalysis.cpp

792 ↗

(On Diff #104005)

Nit: While we're in the area, can this and the other push_front we're touching use emplace_front(const_cast<Function *>(&Fn), this) instead? That would let us skip moving this, which seems to involve nontrivial operations.

[davide]

include/llvm/Analysis/CFLAliasAnalysisUtils.h

10 ↗

(On Diff #104005)

Yes, I suck at writing proper english.

lib/Analysis/CFLAndersAliasAnalysis.cpp

792 ↗

(On Diff #104005)

Indeed. I also dislike a lot templating explicitly when there's no need. Let me try that.

llvm/trunk/include/llvm/Analysis/CFLAliasAnalysisUtils.h

+//=- CFLAliasAnalysisUtils.h - Utilities for CFL Alias Analysis ----*- C++-*-=//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+// \file
+// These are the utilities/helpers used by the CFL Alias Analyses available in
+// tree, i.e. Steensgaard's and Andersens'.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ANALYSIS_CFLALIASANALYSISUTILS_H
+#define LLVM_ANALYSIS_CFLALIASANALYSISUTILS_H
+
+#include "llvm/IR/Function.h"
+#include "llvm/IR/ValueHandle.h"
+
+using namespace llvm;
+
+template <typename AAResult> struct FunctionHandle final : public CallbackVH {
+  FunctionHandle(Function *Fn, AAResult *Result)
+      : CallbackVH(Fn), Result(Result) {
+    assert(Fn != nullptr);
+    assert(Result != nullptr);
+  }
+
+  void deleted() override { removeSelfFromCache(); }
+  void allUsesReplacedWith(Value *) override { removeSelfFromCache(); }
+
+private:
+  AAResult *Result;
+
+  void removeSelfFromCache() {
+    assert(Result != nullptr);
+    auto *Val = getValPtr();
+    Result->evict(cast<Function>(Val));
+    setValPtr(nullptr);
+  }
+};
+
+#endif // LLVM_ANALYSIS_CFLALIASANALYSISUTILS_H

llvm/trunk/include/llvm/Analysis/CFLAndersAliasAnalysis.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_ANALYSIS_CFLANDERSALIASANALYSIS_H
 #define LLVM_ANALYSIS_CFLANDERSALIASANALYSIS_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/Analysis/CFLAliasAnalysisUtils.h"
 #include "llvm/IR/Function.h"
-#include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include <forward_list>
 
 namespace llvm {
 
 class TargetLibraryInfo;
 
 namespace cflaa {
public:
 
   /// Handle invalidation events from the new pass manager.
   /// By definition, this result is stateless and so remains valid.
   bool invalidate(Function &, const PreservedAnalyses &,
                   FunctionAnalysisManager::Invalidator &) {
     return false;
   }
   /// Evict the given function from cache
-  void evict(const Function &Fn);
+  void evict(const Function *Fn);
 
   /// \brief Get the alias summary for the given function
   /// Return nullptr if the summary is not found or not available
   const cflaa::AliasSummary *getAliasSummary(const Function &);
 
   AliasResult query(const MemoryLocation &, const MemoryLocation &);
   AliasResult alias(const MemoryLocation &, const MemoryLocation &);
 
 private:
-  struct FunctionHandle final : public CallbackVH {
-    FunctionHandle(Function *Fn, CFLAndersAAResult *Result)
-        : CallbackVH(Fn), Result(Result) {
-      assert(Fn != nullptr);
-      assert(Result != nullptr);
-    }
-
-    void deleted() override { removeSelfFromCache(); }
-    void allUsesReplacedWith(Value *) override { removeSelfFromCache(); }
-
-  private:
-    CFLAndersAAResult *Result;
-
-    void removeSelfFromCache() {
-      assert(Result != nullptr);
-      auto *Val = getValPtr();
-      Result->evict(*cast<Function>(Val));
-      setValPtr(nullptr);
-    }
-  };
-
   /// \brief Ensures that the given function is available in the cache.
   /// Returns the appropriate entry from the cache.
   const Optional<FunctionInfo> &ensureCached(const Function &);
 
   /// \brief Inserts the given Function into the cache.
   void scan(const Function &);
 
   /// \brief Build summary for a given function
   FunctionInfo buildInfoFrom(const Function &);
 
   const TargetLibraryInfo &TLI;
 
   /// \brief Cached mapping of Functions to their StratifiedSets.
   /// If a function's sets are currently being built, it is marked
   /// in the cache as an Optional without a value. This way, if we
   /// have any kind of recursion, it is discernable from a function
   /// that simply has empty sets.
   DenseMap<const Function *, Optional<FunctionInfo>> Cache;
 
-  std::forward_list<FunctionHandle> Handles;
+  std::forward_list<FunctionHandle<CFLAndersAAResult>> Handles;
 };
 
 /// Analysis pass providing a never-invalidated alias analysis result.
 ///
 /// FIXME: We really should refactor CFL to use the analysis more heavily, and
 /// in particular to leverage invalidation to trigger re-computation.
 class CFLAndersAA : public AnalysisInfoMixin<CFLAndersAA> {
   friend AnalysisInfoMixin<CFLAndersAA>;

llvm/trunk/include/llvm/Analysis/CFLSteensAliasAnalysis.h

 
 #ifndef LLVM_ANALYSIS_CFLSTEENSALIASANALYSIS_H
 #define LLVM_ANALYSIS_CFLSTEENSALIASANALYSIS_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/Analysis/CFLAliasAnalysisUtils.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/Pass.h"
 #include <forward_list>
 
 namespace llvm {
 
AliasResult alias(const MemoryLocation &LocA, const MemoryLocation &LocB) {
     AliasResult QueryResult = query(LocA, LocB);
     if (QueryResult == MayAlias)
       return AAResultBase::alias(LocA, LocB);
 
     return QueryResult;
   }
 
 private:
-  struct FunctionHandle final : public CallbackVH {
-    FunctionHandle(Function *Fn, CFLSteensAAResult *Result)
-        : CallbackVH(Fn), Result(Result) {
-      assert(Fn != nullptr);
-      assert(Result != nullptr);
-    }
-
-    void deleted() override { removeSelfFromCache(); }
-    void allUsesReplacedWith(Value *) override { removeSelfFromCache(); }
-
-  private:
-    CFLSteensAAResult *Result;
-
-    void removeSelfFromCache() {
-      assert(Result != nullptr);
-      auto *Val = getValPtr();
-      Result->evict(cast<Function>(Val));
-      setValPtr(nullptr);
-    }
-  };
-
   const TargetLibraryInfo &TLI;
 
   /// \brief Cached mapping of Functions to their StratifiedSets.
   /// If a function's sets are currently being built, it is marked
   /// in the cache as an Optional without a value. This way, if we
   /// have any kind of recursion, it is discernable from a function
   /// that simply has empty sets.
   DenseMap<Function *, Optional<FunctionInfo>> Cache;
-  std::forward_list<FunctionHandle> Handles;
+  std::forward_list<FunctionHandle<CFLSteensAAResult>> Handles;
 
   FunctionInfo buildSetsFrom(Function *F);
 };
 
 /// Analysis pass providing a never-invalidated alias analysis result.
 ///
 /// FIXME: We really should refactor CFL to use the analysis more heavily, and
 /// in particular to leverage invalidation to trigger re-computation of sets.

llvm/trunk/lib/Analysis/CFLAndersAliasAnalysis.cpp

void CFLAndersAAResult::scan(const Function &Fn) {
   assert(InsertPair.second &&
          "Trying to scan a function that has already been cached");
 
   // Note that we can't do Cache[Fn] = buildSetsFrom(Fn) here: the function call
   // may get evaluated after operator[], potentially triggering a DenseMap
   // resize and invalidating the reference returned by operator[]
   auto FunInfo = buildInfoFrom(Fn);
   Cache[&Fn] = std::move(FunInfo);
-  Handles.push_front(FunctionHandle(const_cast<Function *>(&Fn), this));
+  Handles.emplace_front(const_cast<Function *>(&Fn), this);
 }
 
-void CFLAndersAAResult::evict(const Function &Fn) { Cache.erase(&Fn); }
+void CFLAndersAAResult::evict(const Function *Fn) { Cache.erase(Fn); }
 
 const Optional<CFLAndersAAResult::FunctionInfo> &
 CFLAndersAAResult::ensureCached(const Function &Fn) {
   auto Iter = Cache.find(&Fn);
   if (Iter == Cache.end()) {
     scan(Fn);
     Iter = Cache.find(&Fn);
     assert(Iter != Cache.end());

llvm/trunk/lib/Analysis/CFLSteensAliasAnalysis.cpp

assert(InsertPair.second &&
          "Trying to scan a function that has already been cached");
 
   // Note that we can't do Cache[Fn] = buildSetsFrom(Fn) here: the function call
   // may get evaluated after operator[], potentially triggering a DenseMap
   // resize and invalidating the reference returned by operator[]
   auto FunInfo = buildSetsFrom(Fn);
   Cache[Fn] = std::move(FunInfo);
 
-  Handles.push_front(FunctionHandle(Fn, this));
+  Handles.emplace_front(Fn, this);
 }
 
 void CFLSteensAAResult::evict(Function *Fn) { Cache.erase(Fn); }
 
 /// Ensures that the given function is available in the cache, and returns the
 /// entry.
 const Optional<CFLSteensAAResult::FunctionInfo> &
 CFLSteensAAResult::ensureCached(Function *Fn) {