<div class="gmail_quote">On Tue, Jul 6, 2010 at 2:21 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Jul 6, 2010, at 2:03 PM, Nick Lewycky wrote:</div><br><blockquote type="cite"><div class="gmail_quote">On Tue, Jul 6, 2010 at 1:54 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204);margin:0pt 0pt 0pt 0.8ex;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div>On Jul 6, 2010, at 1:50 PM, Nick Lewycky wrote:</div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204);margin:0pt 0pt 0pt 0.8ex;padding-left:1ex">
<div>> * a new pass named HaltingAttr is added to mark up functions with the halting attribute. If necessary, it queries SCEV for an upper bound on the loop trip count. It is not an SCC pass.<br>
<br>
</div>Why can't this be done in the existing bottom-up pass for inferring this?<br></blockquote><div><br>SCC passes can't depend on FunctionPasses like LoopInfo and SCEV.<br></div></div></blockquote><div><br></div>
</div><div>I think that having the existing SCC pass propagate the bit from callee to caller where possible makes sense. How does a function pass know it is going to visit a callee before a caller? For a function to be "halting" all called functions also have to be known to halt.</div>
</div></div></blockquote><div><br>I tried that. The problem with splitting it into two passes is that the pass doing the local analysis can't safely mark any non-leaf function since the callees haven't been tagged 'halting' yet, and the SCC pass can't mark anything halting since it doesn't have a local analysis to check for loops inside the function itself. Hence they need to be merged.<br>
</div><blockquote class="gmail_quote" style="border-left:1px solid rgb(204, 204, 204);margin:0pt 0pt 0pt 0.8ex;padding-left:1ex"><div style="word-wrap:break-word"><div><div><blockquote type="cite"><div class="gmail_quote">
<div>Also, we can't make use of SCC anyhow because two functions that halt locally but also call each other aren't necessarily halting; they could recurse infinitely. However, having the CallGraphNode* is useful to avoid a linear scan looking for CallInst's as it already has the list.<br>
</div></div></blockquote><div><br></div></div><div>Are you saying that you really have to give up on any non-leaf function?</div></div></div></blockquote><div><br>It's still a bottom-up walk of the call graph, it's just that we can't take advantage of SCCs to infer the property.<br>
</div></div></blockquote><div><br></div></div><div>Is this still useful in practice, or is this realistically something that just gets marked on intrinsics?</div></div></div></blockquote><div><br></div><div>It will only be marked on leaf and close-to-leaf functions. I expect that most functions that get marked readonly will also get a halting attribute on them.</div>
<div><br></div><div>You seem to be really concerned about functions not being marked halting. I don't understand why since none of the optimizations being performed will even apply to the code that's doing real work. The difference is whether or not we delete calls to readonly+nounwind functions whose return value is unused, assuming we don't inline them.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div><div>Also, are potentially blocking operations (mutex lock, 'write', etc) considered to be halting?</div>
</div></div></blockquote><div><br></div><div>No. The other thread could take the lock and then infloop.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word">
<div><div class="im"><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left-width:1px;border-left-style:solid;border-left-color:rgb(204, 204, 204);margin-top:0pt;margin-right:0pt;margin-bottom:0pt;margin-left:0.8ex;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><blockquote type="cite"><div class="gmail_quote"><div>Finally, my patch was missing the part where I actually add HaltingAttr to the list of standard passes. I need to think more about where I want to schedule this; I really want it after the loop optimizations but before the inliner makes its decision. A CGSCCPass would be perfect, were it not for the inability to depend on LoopInfo and SCEV.<br>
</div></div></blockquote><div><br></div></div><div>At the high level, this is the part I'm most concerned with.</div></div></div></blockquote><div><br>Likewise, I hadn't realized how bad it was before sending out the original patch. Once I figure this out I'll send out an patch update.<br>
<br>It's possible that there's no good solution here and we just plain need to run the -haltingattr pass before the inliner and loop passes. That would be sad, but not the end of the world. We could deal with the cause of that (undesirable pass manager constraints) separately. Since CallGraph is in lib/Analysis, it's yet another hard problem.<br>
</div></div></blockquote></div></div><br><div>The other thing that concerns me is that there is a lot of cases that really can't be handled. Iterating over an std::map or another node-based container can't be handled with this. If you have some sort of front-end attributes (like the halting attribute you proposed for clang) or other language semantics that would be useful, then it makes sense.</div>
</div></blockquote><div><br></div><div>Yes, exactly.</div><div><br></div><div>I think I can get a lot of mileage out of smartly choosing which functions to annotate. For starters I don't need to worry about functions that write memory or throw exceptions since there are no optimizations that check the halting attribute which apply to those. I don't need to worry about functions with no loops or obviously bounded loops.</div>
<div><br></div><div>Since we talked about this on IRC, I can tell you that Chandler thinks we should just have a clang flag to add 'halting' on every function and a nohalting attribute. Sean thinks that doesn't work for C++03. Jeff mentioned that C++0x has language stating that loops must end, but they left out recursion (the omission is likely a defect).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div>Some thoughts on the patch:</div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
+++ lib/VMCore/Instruction.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">@@ -410,10 +424,14 @@</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
return false;</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> }</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> case Call:</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
+ // A user-defined function could have undefined behavior, even if it's</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ // readnone nounwind and halting.</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(this)) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ // FIXME: We should special-case some more intrinsics (bswap,</div>
<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ // overflow-checking arithmetic, etc.)</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ return II->doesNotAccessMemory() && II->doesNotThrow() && II->isHalting();</div>
<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ }</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+ return false;</div></div><div><br></div><div>
I don't understand the fixme here.</div>
</div></blockquote><div><br></div><div>It's not safe to speculatively execute something that has undefined behaviour. The fixme was suggesting that there might be intrinsics that are marked readonly (not readnone) and can't ever have undefined behaviour. Since I don't know of any, I've removed the comment.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div>+++ lib/Transforms/Scalar/Sink.cpp<span style="white-space:pre-wrap"> </span>(working copy)</div>
<div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">@@ -234,7 +234,7 @@</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> if (SuccToSinkTo->getUniquePredecessor() != ParentBlock) {</div>
<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> // We cannot sink a load across a critical edge - there may be stores in</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
// other code paths.</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">- if (!Inst->isSafeToSpeculativelyExecute()) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
+ if (Inst->mayReadFromMemory()) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> DEBUG(dbgs() << " *** PUNTING: Wont sink load along critical edge.\n");</div>
<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> return false;</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"> }</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">
<font face="Helvetica"><span style="font-size:medium"><font face="Inconsolata" size="3"><span style="font-size:13px"><br></span></font></span></font></div></div><div>Why is this safe?</div></div></blockquote><div><br></div>
<div>Wow, I misunderstood the code here. Upon re-reading, this doesn't need to change. Thanks.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div>I really don't like the halting pass being a modulepass. This isn't going to fit into the compiler well. It either needs to be a cgsccpass, a function pass, or a combination of the two.</div>
</div></blockquote><div><br></div><div>Done!! I'm pleased to say that I've merged it in with the FunctionAttrs pass! Sorta! There's still a ModulePass but it's just an analysis now. The transformation happens at the right time and the pass ordering isn't disturbed.</div>
<div><br></div><div>Nick</div><div><br></div></div>