[GMR] Fix a long-standing bug in GlobalsModRef where it failed to clear out the per-function modref data structures when functions were deleted or when globals were deleted.

[chandlerc]

Summary

I don't actually know how the global deletion side of this bug hasn't
been hit before, but for the other it just-so-happens that functions
aren't likely to be deleted in the particular part of the LTO pipeline
where we currently enable GMR, so we got lucky.

With this patch, I can self-host with GMR enabled in the normal pass
pipeline!

I was a bit concerned about the compile-time impact of this chang, which
is part of what motivated my prior string of patches to make the
per-function datastructure very dense and fast to walk. With those
changes in place, I can't measure a significant compile time difference
(the difference is around 0.1% which is *way* below the noise) before
and after this patch when building a linked bitcode for all of Clang.

[chandlerc]

Ping.

While I CC'ed Hal on this directly because I know he's interested in AA
stuff, if anyone else has time to review, it would definitely be
appreciated.

[hfinkel]

LGTM.

lib/Analysis/IPA/GlobalsModRef.cpp

241 ↗

(On Diff #30462)

tihs -> this

llvm/trunk/lib/Analysis/IPA/GlobalsModRef.cpp

void addModRefInfoForGlobal(const GlobalValue &GV, ModRefInfo NewMRI) {
     if (!P) {
       P = new AlignedMap();
       Info.setPointer(P);
     }
     auto &GlobalMRI = P->Map[&GV];
     GlobalMRI = ModRefInfo(GlobalMRI | NewMRI);
   }
 
+  /// Clear a global's ModRef info. Should be used when a global is being
+  /// deleted.
+  void eraseModRefInfoForGlobal(const GlobalValue &GV) {
+    if (AlignedMap *P = Info.getPointer())
+      P->Map.erase(&GV);
+  }
+
 private:
   /// All of the information is encoded into a single pointer, with a three bit
   /// integer in the low three bits. The high bit provides a flag for when this
   /// function may read any global. The low two bits are the ModRefInfo. And
   /// the pointer, when non-null, points to a map from GlobalValue to
   /// ModRefInfo specific to that GlobalValue.
   PointerIntPair<AlignedMap *, 3, unsigned, AlignedMapPointerTraits> Info;
 };
struct DeletionCallbackHandle final : CallbackVH {
     GlobalsModRef &GMR;
     std::list<DeletionCallbackHandle>::iterator I;
 
     DeletionCallbackHandle(GlobalsModRef &GMR, Value *V)
         : CallbackVH(V), GMR(GMR) {}
 
     void deleted() override {
       Value *V = getValPtr();
+      if (auto *F = dyn_cast<Function>(V))
+        GMR.FunctionInfos.erase(F);
+
       if (GlobalValue *GV = dyn_cast<GlobalValue>(V)) {
         if (GMR.NonAddressTakenGlobals.erase(GV)) {
           // This global might be an indirect global.  If so, remove it and
-          // remove
-          // any AllocRelatedValues for it.
+          // remove any AllocRelatedValues for it.
           if (GMR.IndirectGlobals.erase(GV)) {
             // Remove any entries in AllocsForIndirectGlobals for this global.
             for (auto I = GMR.AllocsForIndirectGlobals.begin(),
                       E = GMR.AllocsForIndirectGlobals.end();
                  I != E; ++I)
               if (I->second == GV)
                 GMR.AllocsForIndirectGlobals.erase(I);
           }
+
+          // Scan the function info we have collected and remove this global
+          // from all of them.
+          for (auto &FIPair : GMR.FunctionInfos)
+            FIPair.second.eraseModRefInfoForGlobal(*GV);
         }
       }
 
       // If this is an allocation related to an indirect global, remove it.
       GMR.AllocsForIndirectGlobals.erase(V);
 
       // And clear out the handle.
       setValPtr(nullptr);

 
 Pass *llvm::createGlobalsModRefPass() { return new GlobalsModRef(); }
 
 /// AnalyzeGlobals - Scan through the users of all of the internal
 /// GlobalValue's in the program.  If none of them have their "address taken"
 /// (really, their address passed to something nontrivial), record this fact,
 /// and record the functions that they are used directly in.
 void GlobalsModRef::AnalyzeGlobals(Module &M) {
+  SmallPtrSet<Function *, 64> TrackedFunctions;
   for (Function &F : M)
     if (F.hasLocalLinkage())
       if (!AnalyzeUsesOfPointer(&F)) {
         // Remember that we are tracking this global.
         NonAddressTakenGlobals.insert(&F);
+        TrackedFunctions.insert(&F);
         Handles.emplace_front(*this, &F);
         Handles.front().I = Handles.begin();
         ++NumNonAddrTakenFunctions;
       }
 
   SmallPtrSet<Function *, 64> Readers, Writers;
   for (GlobalVariable &GV : M.globals())
     if (GV.hasLocalLinkage()) {
       if (!AnalyzeUsesOfPointer(&GV, &Readers,
                                 GV.isConstant() ? nullptr : &Writers)) {
         // Remember that we are tracking this global, and the mod/ref fns
         NonAddressTakenGlobals.insert(&GV);
         Handles.emplace_front(*this, &GV);
         Handles.front().I = Handles.begin();
 
-        for (Function *Reader : Readers)
+        for (Function *Reader : Readers) {
+          if (TrackedFunctions.insert(Reader).second) {
+            Handles.emplace_front(*this, Reader);
+            Handles.front().I = Handles.begin();
+          }
           FunctionInfos[Reader].addModRefInfoForGlobal(GV, MRI_Ref);
+        }
 
         if (!GV.isConstant()) // No need to keep track of writers to constants
-          for (Function *Writer : Writers)
+          for (Function *Writer : Writers) {
+            if (TrackedFunctions.insert(Writer).second) {
+              Handles.emplace_front(*this, Writer);
+              Handles.front().I = Handles.begin();
+            }
             FunctionInfos[Writer].addModRefInfoForGlobal(GV, MRI_Mod);
+          }
         ++NumNonAddrTakenGlobalVars;
 
         // If this global holds a pointer type, see if it is an indirect global.
         if (GV.getType()->getElementType()->isPointerTy() &&
             AnalyzeIndirectGlobalMemory(&GV))
           ++NumIndirectGlobalVars;
       }
       Readers.clear();