<div dir="ltr">Thanks for the review!<div><br></div><div>I've addressed all of these issues except for the map changes. I completely agree with all of your suggestions, but I'd rather keep this as just moving the code into the callback in this patch. I'll fix the code to actually be cleaner in a follow-up patch. I'm hoping to move it to a DenseMap anyways since we're iterating it frequently and that is especially slow for a std::map.</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Jul 21, 2015 at 4:43 PM Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> On 2015-Jul-18, at 02:30, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br>
><br>
> chandlerc created this revision.<br>
> chandlerc added a subscriber: llvm-commits.<br>
><br>
> This is technically more expensive, but in benchmarking some LTO runs,<br>
> it seems unlikely to even be above the noise floor. The only way I was<br>
> able to measure the performance of GMR at all was to run nothing else<br>
> but this one analysis on a linked clang bitcode file. The call graph<br>
> analysis still took 5x more time than GMR, and this change at most made<br>
> GMR 2% slower (this is well within the noise, so its hard for me to be<br>
> sure that this is an actual change). However, in a real LTO run over the<br>
> same bitcode, the GMR run takes so little time that the pass timers<br>
> don't measure it.<br>
><br>
> With this, I can remove the last update API from the AliasAnalysis<br>
> interface, but I'll actually remove the interface hook point in<br>
> a follow-up commit.<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11324&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=hOKT6HIACu0UGstIi_95Nf52KQKPXCTkoQCCj9sqUFc&s=lray7mCpqWriPgpwNjpjubwfOAAJxwyTBBim0xaBcA8&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11324</a><br>
><br>
> Files:<br>
> lib/Analysis/IPA/GlobalsModRef.cpp<br>
<br>
I find your benchmarking convincing. LGTM with some nitpicks/<br>
suggestions inline (maybe some of them are from the copy/paste,<br>
but I saw them, so...).<br>
<br>
> Index: lib/Analysis/IPA/GlobalsModRef.cpp<br>
> ===================================================================<br>
> --- lib/Analysis/IPA/GlobalsModRef.cpp<br>
> +++ lib/Analysis/IPA/GlobalsModRef.cpp<br>
> @@ -214,6 +218,46 @@<br>
><br>
> Pass *llvm::createGlobalsModRefPass() { return new GlobalsModRef(); }<br>
><br>
> +struct GlobalsModRef::DeletionCallbackHandle : CallbackVH {<br>
<br>
Should you use `final` here?<br>
<br>
> + GlobalsModRef &GMR;<br>
> + std::list<DeletionCallbackHandle>::iterator I;<br>
<br>
Just to document it, I was a little worried at first about the memory<br>
impact of this, since if you include the linked list nodes, each<br>
`DeletionCallbackHandle` costs something like 8 pointers.<br>
<br>
However, IIUC, there's (at most) one of these for each<br>
`GlobalVariable`, so I can't imagine this actually blowing up in any<br>
measurable way. I think I've just spent too much time in MC land.<br>
<br>
> +<br>
> + DeletionCallbackHandle(GlobalsModRef &GMR, Value *V)<br>
> + : CallbackVH(V), GMR(GMR) {}<br>
> +<br>
> + virtual void deleted() {<br>
<br>
Should you use `override` here?<br>
<br>
> + Value *V = getValPtr();<br>
> + if (GlobalValue *GV = dyn_cast<GlobalValue>(V)) {<br>
> + if (GMR.NonAddressTakenGlobals.erase(GV)) {<br>
> + // This global might be an indirect global. If so, remove it and remove<br>
> + // any AllocRelatedValues for it.<br>
> + if (GMR.IndirectGlobals.erase(GV)) {<br>
> + // Remove any entries in AllocsForIndirectGlobals for this global.<br>
> + for (std::map<const Value *, const GlobalValue *>::iterator<br>
<br>
Maybe you could use `auto` here, but I guess this makes it clear that<br>
erasing doesn't invalidate `E`?<br>
<br>
> + I = GMR.AllocsForIndirectGlobals.begin(),<br>
> + E = GMR.AllocsForIndirectGlobals.end();<br>
> + I != E;) {<br>
> + if (I->second == GV) {<br>
> + GMR.AllocsForIndirectGlobals.erase(I++);<br>
<br>
Do MSVC13 and GCC 4.7 support `erase()`'s return value in C++11?<br>
<br>
I = GMR.AllocsForIndirectGlobals.erase(I);<br>
<br>
IMO, it makes it more clear that `I` is updated correctly.<br>
<br>
> + } else {<br>
> + ++I;<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> + }<br>
> +<br>
> + // Otherwise, if this is an allocation related to an indirect global, remove<br>
> + // it.<br>
<br>
Doesn't seem like "Otherwise" is correct, since you're doing this<br>
always.<br>
<br>
> + GMR.AllocsForIndirectGlobals.erase(V);<br>
> +<br>
> + // And clear out the handle.<br>
> + setValPtr(nullptr);<br>
> + GMR.Handles.erase(I);<br>
> + // This object is now destroyed!<br>
> + }<br>
> +};<br>
> +<br>
> /// AnalyzeGlobals - Scan through the users of all of the internal<br>
> /// GlobalValue's in the program. If none of them have their "address taken"<br>
> /// (really, their address passed to something nontrivial), record this fact,<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>