<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, Aug 7, 2016 at 11:56 PM Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Aug 7, 2016 at 11:40 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><div dir="ltr">On Sun, Aug 7, 2016 at 10:45 PM Sean Silva via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Author: silvas<br>
Date: Mon Aug 8 00:38:01 2016<br>
New Revision: 277980<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=277980&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=277980&view=rev</a><br>
Log:<br>
[PM] Invalidate CallGraphAnalysis because it holds AssertingVH<br>
<br>
This is essentially PR28400. The fix here is similar to that implemented<br>
in r274656.<br></blockquote><div><br></div></span><div>This patch, without a test case, is very hard to understand. It doesn't seem to make sense in isolation.</div><div><br></div><div>Maybe this makes more sense in combination with the large patch around invalidation you mailed out? If so, I don't think it should just be committed.</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>No, this is unrelated to that patch. This is related to the thread "Should analyses be able to hold AssertingVH to IR? (related to PR28400)" linked from r274656 which was mentioned in the commit message.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>If this makes sense outside that patch, I would expect a test case? Maybe discuss this before committing it?</div><span><div> </div></span></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div>I described this workaround in that thread and there didn't seem to be any objections to doing that for now.</div></div></div></div></div></blockquote><div><br></div><div>I don't think that really counts as code review, nor does it really help other readers of commits who may not have read the thread you are mentioning. Assuming everyone has read that thread doesn't seem reasonable IMO.</div><div><br></div><div>Perhaps revert these workarounds and send out a patch for review with them, and include a succinct summary of what the plan is here?<br><br>Again, I'm not trying to say this is necessarily the wrong approach, I just think you need to go through the right steps here to make sure folks are aware and understand the implications.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><br></div><div>-- Sean Silva</div></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=277980&r1=277979&r2=277980&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=277980&r1=277979&r2=277980&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Mon Aug 8 00:38:01 2016<br>
@@ -1271,6 +1271,11 @@ ReversePostOrderFunctionAttrsPass::run(M<br>
auto &CG = AM.getResult<CallGraphAnalysis>(M);<br>
<br>
bool Changed = deduceFunctionAttributeInRPO(M, CG);<br>
+<br>
+ // CallGraphAnalysis holds AssertingVH and must be invalidated eagerly so<br>
+ // that other passes don't delete stuff from under it.<br>
+ AM.invalidate<CallGraphAnalysis>(M);<br>
+<br>
if (!Changed)<br>
return PreservedAnalyses::all();<br>
PreservedAnalyses PA;<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div></div>
</blockquote></div></div></div></blockquote></div></div>