<div dir="ltr">After a long discussion with James, I mostly think this is the right approach, but it is pretty badly incomplete.<div><br></div><div>Currently, combinations of mergefuncs and norecurse can cause miscompiles of programs. We need to really carefully audit the few passes that actually introduce call edges to fix all of the ways they can introduce recursion. It's a bit alarming that all of this infrastructure is on and being used by globalopt when that audit hasn't taken place, but I trust James will get to it (real soon i hope?)</div><div><br></div><div>We also need *something* to annotate strlen and other things with, otherwise it becomes obnoxiously hard to actually infer norecurse anywhere and we're burning *considerable* infrastructure and compile time trying to do so. It's like noreentrant but not quite that exact concept. =/ I dunno what to call it. But unless we can reasonably mark memcpy and strlen and friends as not calling back out to user code, we're in the awful place of doing lots of work to try to infer norecurse but failing for every single function that does a struct copy or uses std::string. =/</div><div><br></div><div>Anyways, I'll rip norecurse out of this patch clearly, as it implies far more than I thought. I'm also struggling to find a way to actually represent the inference we have in a clean way (the finalization trick is... not going to work).</div><div><br></div><div>-Chandler</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 22, 2015 at 11:04 AM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
I agree w/James perspective here. In particular, I feel like the
issue with norecurse is how we chose to specify it and not the
notion itself or the implementation thereof. We should raise some
of the issues on llvm-dev and hammer through them, but I don't think
we need to revert anything in the meantime. This doesn't feel like
a major problem or the fundamentally wrong approach.</div><div text="#000000" bgcolor="#FFFFFF"><br>
<br>
Philip</div><div text="#000000" bgcolor="#FFFFFF"><br>
<br>
<div>On 12/21/2015 11:51 AM, James Molloy
via llvm-commits wrote:<br>
</div>
<blockquote type="cite">Hey Chandler,<br>
<br>
I'll think this through and give a more reasoned reply in the
morning. However I would just say that I'm not opposed to an
alternative implementation that meets the same ends. <br>
<br>
I did put a lot of effort into this current solution though and it
is *significantly* less broken than what was in LLVM before (main
is non recursive, unconditionally!!). It currently gives
nontrivial speed ups to workloads we care about, so ripping it all
out before designing something new (likely taking at least a month
given the season) isn't massively appealing.<br>
<br>
Cheers,<br>
<br>
James<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, 21 Dec 2015 at 19:40, Chandler Carruth
<<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@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">I'm getting the strong indication that
norecurse isn't actually a good idea, and hasn't been
hammered through enough.
<div><br>
</div>
<div>It turns out that I also need to build brand new
infrastructure to keep supporting it.</div>
<div><br>
</div>
<div>I'm ripping it out of this patch clearly, but I am
wondering whether we want to rip it out entirely and start
a new discussion on the actual problem James is trying to
solve and the best way to go about it.</div>
<div><br>
</div>
<div>Note that, for example, if strlen and memcpy can't be
marked as norecurse (and I agree that they can't in a
strict sense as it is conforming to implement both of them
as recursive algorithms) then *any* inference of norecurse
in LLVM is actually broken today because we may add calls
to non-norecurse functions (namely memcpy) at a later
point. Given that, I think the spec for norecurse is too
narrow to be a terribly useful property. I suspect that
James will need to use some other strategy to get the
useful optimizations he's after (which center around
*main* not being recursive, a very different beast).</div>
</div>
<div dir="ltr">
<div><br>
</div>
<div>-Chandler</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, Dec 21, 2015 at 10:09 AM Mehdi Amini
<<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.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="auto">
<div><br>
<br>
Sent from my iPhone</div>
</div>
<div dir="auto">
<div><br>
On Dec 21, 2015, at 12:07 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk" target="_blank"><a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a></a>>
wrote:<br>
<br>
</div>
<blockquote type="cite">
<div>
<div dir="ltr">Hi Mehdi,
<div><br>
</div>
<div>> For strlen: you’re right that it is not
correct in the absolute, but for what we use the
information I’m not sure it matters. We’re
interested in the fact that it won’t call back
in the user code in any way, so that we can
infer the norecurse on the callers.<br>
</div>
<div><br>
</div>
<div>I understand where you're coming from, but I
think second-guessing how an attribute may be
used is a dangerous route to go down.
"norecurse" could be used for something else in
the future, so unless we have a good reason it
would be good not to make assumptions.</div>
<div><br>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
</div>
<div dir="auto">
<div>Agree, but if we go on the "safe way" now it means
we should either redefine the norecurse attribute
(like the almostreadnone for example) or have another
one.</div>
</div>
<div dir="auto">
<div><br>
</div>
<div>Mehdi</div>
</div>
<div dir="auto">
<div><br>
</div>
<div><br>
</div>
<br>
<blockquote type="cite">
<div>
<div dir="ltr">
<div>James</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr">On Mon, 21 Dec 2015 at 10:06 James
Molloy via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank"><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a></a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">jmolloy accepted this
revision.<br>
jmolloy added a comment.<br>
This revision is now accepted and ready to land.<br>
<br>
Hi Chandler,<br>
<br>
Thanks for taking the time to do this.<br>
<br>
I agree with Philip - while I like that you've
added extra norecurse attributes, I don't like
that it's smuggled in as part of this patch. I'd
highly prefer this patch to be NFC and then a
simple update patch adding the norecurse
attributes.<br>
<br>
Apart from that and the specific comment below,
it LGTM.<br>
<br>
Cheers,<br>
<br>
James<br>
<br>
<br>
================<br>
Comment at:
include/llvm/Transforms/IPO/InferFunctionAttrs.h:25<br>
@@ +24,3 @@<br>
+/// A pass which infers function attributes
from the names and signatures of<br>
+/// function declarations in a module..<br>
+class InferFunctionAttrsPass {<br>
----------------<br>
Double period.<br>
<br>
================<br>
Comment at:
include/llvm/Transforms/IPO/InferFunctionAttrs.h:26<br>
@@ +25,3 @@<br>
+/// function declarations in a module..<br>
+class InferFunctionAttrsPass {<br>
+public:<br>
----------------<br>
Unrelated: Damn, new style passes look so much
nicer!<br>
<br>
<br>
<a href="http://reviews.llvm.org/D15676" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15676</a><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" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>
<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>
</pre>
</blockquote>
<br>
</div></blockquote></div>