[PATCH] D11453: [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.

[Chandler Carruth]

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.

On Thu, Jul 23, 2015 at 2:04 AM Chandler Carruth <chandlerc at gmail.com>
wrote:

> chandlerc created this revision.
> chandlerc added a reviewer: hfinkel.
> chandlerc added a subscriber: llvm-commits.
>
> 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.
>
> http://reviews.llvm.org/D11453
>
> Files:
>   lib/Analysis/IPA/GlobalsModRef.cpp
>
> Index: lib/Analysis/IPA/GlobalsModRef.cpp
> ===================================================================
> --- lib/Analysis/IPA/GlobalsModRef.cpp
> +++ lib/Analysis/IPA/GlobalsModRef.cpp
> @@ -180,6 +180,13 @@
>      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
> @@ -215,19 +222,26 @@
>
>      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 tihs
> global
> +          // from all of them.
> +          for (auto &FIPair : GMR.FunctionInfos)
> +            FIPair.second.eraseModRefInfoForGlobal(*GV);
>          }
>        }
>
> @@ -361,11 +375,13 @@
>  /// (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;
> @@ -381,12 +397,22 @@
>          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.
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150725/113f70d3/attachment.html>