<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="SV" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">I’ve analysed this a little bit more.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">In CGPassManager::RunPassOnSCC we have this code:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> …<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> Changed |= FPP->runOnFunction(*F);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> …<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> // The function pass(es) modified the IR, they may have clobbered the<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> // callgraph.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> if (Changed && CallGraphUpToDate) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> DEBUG(dbgs() << "CGSCCPASSMGR: Pass Dirtied SCC: "<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> << P->getPassName() << '\n');<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> CallGraphUpToDate = false;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">But the thing is that even though InstCombine is doing changes (DCE:ing a call), it won’t return true. So the CGPassManager will not
set CallGraphUpToDate = false.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">(well, I know that InstCombine also says that it preserves CFG… but looks like CGPassManager doesn’t trust function passes when running
them this way)<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">What I noticed was that AddReachableCodeToWorklist (in InstructionCombining.cpp) does not always set MadeIRChange = true even if optimisations
are done.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">This happens at least for the
<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> // DCE instruction if trivially dead.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">and the<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> // ConstantProp instruction if trivially constant.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">optimisations.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">If I add<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"> MadeIRChange = true;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">when doing those optimisations, then the CGPassManager will set CallGraphUpToDate = false, and later refresh the CallGraph if needed
again.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">So with such a modification to AddReachableCodeToWorklist we no longer get the assert.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">I can prepare a patch for this next week if it sounds reasonable (and no one else is faster than me… my work week has actually ended
already..).<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">Regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US">Björn<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif"> David Blaikie [mailto:dblaikie@gmail.com]
<br>
<b>Sent:</b> den 16 juni 2017 02:17<br>
<b>To:</b> Björn Pettersson A <bjorn.a.pettersson@ericsson.com>; Mikael Holmén <mikael.holmen@ericsson.com>; llvm-commits@lists.llvm.org; Chandler Carruth <chandlerc@gmail.com><br>
<b>Subject:</b> Re: [llvm] r305245 - Inliner: Don't remove calls to readnone+nounwind (but not always_inline) functions in the AlwaysInliner<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Looks like this has exposed a bug in instcombine where instcombine will remove a trivially dead call which can invalidate the call graph (yet instcombine claims to preserve the call graph).<br>
<br>
A trivial change to avoid removing such calls seems to be tripping up some things for me... I think it's because a bunch of instcombine at least relies on removing trivially dead calls to intrinsics, perhaps.<br>
<br>
hrm... <o:p></o:p></p>
<div>
<div>
<p class="MsoNormal">On Thu, Jun 15, 2017 at 6:40 AM Björn Pettersson A <<a href="mailto:bjorn.a.pettersson@ericsson.com">bjorn.a.pettersson@ericsson.com</a>> wrote:<o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm">
<p class="MsoNormal">A smaller reproducer:<br>
<br>
;-------------------------------------------------------------------------------<br>
; opt -S -functionattrs -inline -instcombine -o -<br>
<br>
; Function Attrs: noinline norecurse nounwind readnone<br>
define void @getPageAddr() #0 {<br>
entry:<br>
%call = call i32 @cmAddr()<br>
ret void<br>
}<br>
<br>
; Function Attrs: noinline norecurse nounwind readnone<br>
define internal i32 @cmAddr() #0 {<br>
entry:<br>
ret i32 5<br>
}<br>
<br>
attributes #0 = { noinline norecurse nounwind readnone }<br>
;-------------------------------------------------------------------------------<br>
<br>
<br>
Note that if I execute this using<br>
opt -S -functionattrs -inline -print-callgraph -instcombine -o -<br>
instead of<br>
opt -S -functionattrs -inline -instcombine -o -<br>
the assert won't trigger (although, cmAddr is not removed and I think it asserts when trying to remove that function).<br>
<br>
Regards,<br>
Björn<br>
<br>
> -----Original Message-----<br>
> From: Mikael Holmén<br>
> Sent: den 15 juni 2017 13:02<br>
> To: David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>>;
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> Cc: Björn Pettersson A <<a href="mailto:bjorn.a.pettersson@ericsson.com" target="_blank">bjorn.a.pettersson@ericsson.com</a>><br>
> Subject: Re: [llvm] r305245 - Inliner: Don't remove calls to<br>
> readnone+nounwind (but not always_inline) functions in the AlwaysInliner<br>
><br>
> Hi David,<br>
><br>
> With this commit, opt crashes when doing:<br>
><br>
> opt -S -functionattrs -inline -instcombine -o - callgraph.ll<br>
><br>
> opt: ../include/llvm/Analysis/CallGraph.h:174:<br>
> llvm::CallGraphNode::~CallGraphNode(): Assertion `NumReferences == 0 &&<br>
> "Node deleted while references remain"' failed.<br>
><br>
> Regards,<br>
> Mikael<br>
><br>
> On 06/13/2017 01:01 AM, David Blaikie via llvm-commits wrote:<br>
> > Author: dblaikie<br>
> > Date: Mon Jun 12 18:01:17 2017<br>
> > New Revision: 305245<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=305245&view=rev" target="_blank">
http://llvm.org/viewvc/llvm-project?rev=305245&view=rev</a><br>
> > Log:<br>
> > Inliner: Don't remove calls to readnone+nounwind (but not always_inline)<br>
> functions in the AlwaysInliner<br>
> ><br>
> > Modified:<br>
> > llvm/trunk/lib/Transforms/IPO/Inliner.cpp<br>
> > llvm/trunk/test/Transforms/Inline/always-inline.ll<br>
> ><br>
> > Modified: llvm/trunk/lib/Transforms/IPO/Inliner.cpp<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/lib/Transforms/IPO/Inliner.cpp?rev=305245&r1=305244&<br>
> r2=305245&view=diff<br>
> ><br>
> ==========================================================<br>
> ====================<br>
> > --- llvm/trunk/lib/Transforms/IPO/Inliner.cpp (original)<br>
> > +++ llvm/trunk/lib/Transforms/IPO/Inliner.cpp Mon Jun 12 18:01:17 2017<br>
> > @@ -523,6 +523,16 @@ inlineCallsImpl(CallGraphSCC &SCC, CallG<br>
> > if (!Callee || Callee->isDeclaration())<br>
> > continue;<br>
> ><br>
> > + // FIXME for new PM: because of the old PM we currently generate<br>
> ORE and<br>
> > + // in turn BFI on demand. With the new PM, the ORE dependency<br>
> should<br>
> > + // just become a regular analysis dependency.<br>
> > + OptimizationRemarkEmitter ORE(Caller);<br>
> > +<br>
> > + // If the policy determines that we should inline this function,<br>
> > + // delete the call instead.<br>
> > + if (!shouldInline(CS, GetInlineCost, ORE))<br>
> > + continue;<br>
> > +<br>
> > // If this call site is dead and it is to a readonly function, we should<br>
> > // just delete the call instead of trying to inline it, regardless of<br>
> > // size. This happens because IPSCCP propagates the result out of the<br>
> > @@ -548,15 +558,6 @@ inlineCallsImpl(CallGraphSCC &SCC, CallG<br>
> > // Get DebugLoc to report. CS will be invalid after Inliner.<br>
> > DebugLoc DLoc = CS.getInstruction()->getDebugLoc();<br>
> > BasicBlock *Block = CS.getParent();<br>
> > - // FIXME for new PM: because of the old PM we currently generate<br>
> ORE and<br>
> > - // in turn BFI on demand. With the new PM, the ORE dependency<br>
> should<br>
> > - // just become a regular analysis dependency.<br>
> > - OptimizationRemarkEmitter ORE(Caller);<br>
> > -<br>
> > - // If the policy determines that we should inline this function,<br>
> > - // try to do so.<br>
> > - if (!shouldInline(CS, GetInlineCost, ORE))<br>
> > - continue;<br>
> ><br>
> > // Attempt to inline the function.<br>
> > using namespace ore;<br>
> ><br>
> > Modified: llvm/trunk/test/Transforms/Inline/always-inline.ll<br>
> > URL: <a href="http://llvm.org/viewvc/llvm-" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/test/Transforms/Inline/always-<br>
> inline.ll?rev=305245&r1=305244&r2=305245&view=diff<br>
> ><br>
> ==========================================================<br>
> ====================<br>
> > --- llvm/trunk/test/Transforms/Inline/always-inline.ll (original)<br>
> > +++ llvm/trunk/test/Transforms/Inline/always-inline.ll Mon Jun 12 18:01:17<br>
> 2017<br>
> > @@ -305,3 +305,14 @@ entry:<br>
> > ret void<br>
> > ; CHECK: ret void<br>
> > }<br>
> > +<br>
> > +define void @inner14() readnone nounwind {<br>
> > +; CHECK: define void @inner14<br>
> > + ret void<br>
> > +}<br>
> > +<br>
> > +define void @outer14() {<br>
> > +; CHECK: call void @inner14<br>
> > + call void @inner14()<br>
> > + ret void<br>
> > +}<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" target="_blank">
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
> ><o:p></o:p></p>
</blockquote>
</div>
</div>
</div>
</div>
</body>
</html>