<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><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">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">james@jamesmolloy.co.uk</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">llvm-commits@lists.llvm.org</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>