<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 November 2015 at 14:50, Sanjoy Das <span dir="ltr"><<a href="mailto:sanjoy@playingwithpointers.com" target="_blank">sanjoy@playingwithpointers.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">sanjoy added a subscriber: nlewycky.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:445<br>
@@ -444,2 +444,3 @@<br>
</span><span class="">       CallSite::arg_iterator B = CS.arg_begin(), E = CS.arg_end();<br>
-      for (CallSite::arg_iterator A = B; A != E; ++A, ++AI) {<br>
+      bool InVarArgSection = false;<br>
+      for (CallSite::arg_iterator A = B; A != E; ++A) {<br>
</span>----------------<br>
<span class="">reames wrote:<br>
> I would rather we just gave up on VarArg callsites unless you have a compelling reason we need to handle them here.  Return a conservative answer, and let someone else refine it later.<br>
><br>
> p.s. Does the other patch have the same issue?<br>
</span>I didn't add the code, so I'm not in a position to judge whether there is a compelling reason for this optimization or not. :)<br>
<br>
Moreover, bailing out unconditionally on varargs functions breaks `test/Transforms/FunctionAttrs/readattrs.ll` (added by @nlewycky in Jul 2013), and I'd rather not pessimize transforms that have in-tree tests.<br></blockquote><div><br></div><div>It's long ago enough that I'm not 100% sure, but IIRC the varargs case got the first test because it happened to me late in testing the original patch, before I started writing a proper testcase file. I don't think varargs are really important to optimize, but it would match the string literals in printf-like functions, and it would be nice to get that right.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> p.s. Does the other patch have the same issue?<br>
<br>
</span>If you mean D14306, then I don't think so -- that change is fine as far as I can tell.<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D14350" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14350</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>