[PATCH] D11324: [PM/AA] Replace the only use of the AliasAnalysis::deleteValue API (in GlobalsModRef) with CallbackVHs that trigger the same behavior.

[Duncan P. N. Exon Smith]

> On 2015-Jul-18, at 02:30, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> chandlerc created this revision.
> chandlerc added a subscriber: llvm-commits.
> 
> This is technically more expensive, but in benchmarking some LTO runs,
> it seems unlikely to even be above the noise floor. The only way I was
> able to measure the performance of GMR at all was to run nothing else
> but this one analysis on a linked clang bitcode file. The call graph
> analysis still took 5x more time than GMR, and this change at most made
> GMR 2% slower (this is well within the noise, so its hard for me to be
> sure that this is an actual change). However, in a real LTO run over the
> same bitcode, the GMR run takes so little time that the pass timers
> don't measure it.
> 
> With this, I can remove the last update API from the AliasAnalysis
> interface, but I'll actually remove the interface hook point in
> a follow-up commit.
> 
> http://reviews.llvm.org/D11324
> 
> Files:
>  lib/Analysis/IPA/GlobalsModRef.cpp

I find your benchmarking convincing.  LGTM with some nitpicks/
suggestions inline (maybe some of them are from the copy/paste,
but I saw them, so...).

> Index: lib/Analysis/IPA/GlobalsModRef.cpp
> ===================================================================
> --- lib/Analysis/IPA/GlobalsModRef.cpp
> +++ lib/Analysis/IPA/GlobalsModRef.cpp
> @@ -214,6 +218,46 @@
>  
>  Pass *llvm::createGlobalsModRefPass() { return new GlobalsModRef(); }
>  
> +struct GlobalsModRef::DeletionCallbackHandle : CallbackVH {

Should you use `final` here?

> +  GlobalsModRef &GMR;
> +  std::list<DeletionCallbackHandle>::iterator I;

Just to document it, I was a little worried at first about the memory
impact of this, since if you include the linked list nodes, each
`DeletionCallbackHandle` costs something like 8 pointers.

However, IIUC, there's (at most) one of these for each
`GlobalVariable`, so I can't imagine this actually blowing up in any
measurable way.  I think I've just spent too much time in MC land.

> +
> +  DeletionCallbackHandle(GlobalsModRef &GMR, Value *V)
> +      : CallbackVH(V), GMR(GMR) {}
> +
> +  virtual void deleted() {

Should you use `override` here?

> +    Value *V = getValPtr();
> +    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.
> +        if (GMR.IndirectGlobals.erase(GV)) {
> +          // Remove any entries in AllocsForIndirectGlobals for this global.
> +          for (std::map<const Value *, const GlobalValue *>::iterator

Maybe you could use `auto` here, but I guess this makes it clear that
erasing doesn't invalidate `E`?

> +                   I = GMR.AllocsForIndirectGlobals.begin(),
> +                   E = GMR.AllocsForIndirectGlobals.end();
> +               I != E;) {
> +            if (I->second == GV) {
> +              GMR.AllocsForIndirectGlobals.erase(I++);

Do MSVC13 and GCC 4.7 support `erase()`'s return value in C++11?

    I = GMR.AllocsForIndirectGlobals.erase(I);

IMO, it makes it more clear that `I` is updated correctly.

> +            } else {
> +              ++I;
> +            }
> +          }
> +        }
> +      }
> +    }
> +
> +    // Otherwise, if this is an allocation related to an indirect global, remove
> +    // it.

Doesn't seem like "Otherwise" is correct, since you're doing this
always.

> +    GMR.AllocsForIndirectGlobals.erase(V);
> +
> +    // And clear out the handle.
> +    setValPtr(nullptr);
> +    GMR.Handles.erase(I);
> +    // This object is now destroyed!
> +  }
> +};
> +
>  /// 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,