<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Dec 20, 2015, at 4:06 PM, Philip Reames <<a href="mailto:listmail@philipreames.com" class="">listmail@philipreames.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type" class="">
<div text="#000000" bgcolor="#FFFFFF" class="">
<div class="moz-cite-prefix">For some reason, I can't seem to log in
to phabricator at the moment... A couple of comments below.<br class="">
<br class="">
"InferFunctionAttrs" - bikeshed wise, this seems like a very
generic, non-discriptive name. Possibly:
InferFunctionAttrsFromTLI? Or something which otherwise gives a
clue as to what we're infering from?<br class="">
<br class="">
This really seems like it should somehow be a tablegen file. I
know you're just moving code around, but the long list of hand
written code for each function smells. Particularly the type
checking of arguments after TLI already matched to an enum...<br class="">
<br class="">
Stale comment in "inferAllPrototypeAttributes".<br class="">
<br class="">
I'm uncomfortable with the addition of the various no-recurse
attributes. I'm not saying your choices are wrong, but mixing
them in with a large code motion patch makes them hard to review.
There's also a question of what we're assuming about the runtime
environment. After all, there is a perfectly legal (if unlikely)
recursive implementation of strlen.<br class=""></div></div></div></blockquote><div><br class=""></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.</div><div><br class=""></div><div>— </div><div>Mehdi</div><div><br class=""></div><div><br class=""></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div text="#000000" bgcolor="#FFFFFF" class=""><div class="moz-cite-prefix">
<br class="">
Otherwise, LGTM.<br class="">
<br class="">
Philip <br class="">
<br class="">
<br class="">
<br class="">
On 12/20/2015 03:13 AM, Chandler Carruth via llvm-commits wrote:<br class="">
</div>
<blockquote cite="mid:6c91c054a01609ae91b685ea76ca9403@localhost.localdomain" type="cite" class="">
<pre wrap="" class="">chandlerc updated this revision to Diff 43324.
chandlerc added a comment.
Finish wiring up the new pass manager for inferring function attrs, and test it.
<a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D15676">http://reviews.llvm.org/D15676</a>
Files:
include/llvm/InitializePasses.h
include/llvm/Transforms/IPO/InferFunctionAttrs.h
lib/Passes/PassBuilder.cpp
lib/Passes/PassRegistry.def
lib/Transforms/IPO/CMakeLists.txt
lib/Transforms/IPO/FunctionAttrs.cpp
lib/Transforms/IPO/IPO.cpp
lib/Transforms/IPO/InferFunctionAttrs.cpp
lib/Transforms/IPO/PassManagerBuilder.cpp
test/Transforms/FunctionAttrs/2009-01-04-Annotate.ll
test/Transforms/FunctionAttrs/annotate-1.ll
test/Transforms/InferFunctionAttrs/annotate.ll
test/Transforms/InstCombine/strto-1.ll
</pre>
<br class="">
<fieldset class="mimeAttachmentHeader"></fieldset>
<br class="">
<pre wrap="" class="">_______________________________________________
llvm-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br class="">
</div>
</div></blockquote></div><br class=""></body></html>