<div dir="ltr">On 11 November 2013 14:11, Duncan Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">



<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for the new review!  Updated patch attached.  See comments inline.<br>
<div><br>
On Nov 11, 2013, at 12:50 PM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
<br>
> +//===-- CodeGen/RuntimeLibcalls.h - Runtime Library Calls --------*-<br>
> C++ -*-===//<br>
><br>
> Nice cleanup, but unrelated. Please commit it independently.<br>
<br>
</div>Removed it.<br>
<div><br>
> +                        const std::vector<llvm::StringRef> &Libcalls,<br>
><br>
> Can you use an ArrayRef?<br>
<br>
</div>Fixed.<br>
<div><br>
> +  std::sort(Libcalls.begin(), Libcalls.end());<br>
> +  Libcalls.erase(std::unique(Libcalls.begin(), Libcalls.end()),<br>
> +                 Libcalls.end());<br>
><br>
> What is the size of this std::vector? Would a StringSet be better?<br>
<br>
</div>I see ~600 entries for RTLIB::Libcall and LibFunc::Func combined.  I don’t have a good sense of how StringSet and std::vector compare at this size, so I’ll defer to your judgement.  I’ve left it as std::vector for now.<br>




<br>
Typically I default to std::vector until I see a performance problem, and <<a href="http://llvm.org/docs/ProgrammersManual.html#dss-sortedvectorset" target="_blank">http://llvm.org/docs/ProgrammersManual.html#dss-sortedvectorset</a>> suggested my pattern was okay.  However, I missed the discussion of StringSet/StringMap — should I use StringSet by default for strings?<br>




<div><br>
> +; RUN: FileCheck <%t3 %s -check-prefix=LIBC<br>
> +; RUN: FileCheck <%t3 %s -check-prefix=RT<br>
> +; RUN: FileCheck <%t3 %s -check-prefix=BOTH<br>
> +; RUN: FileCheck <%t3 %s -check-prefix=NEITHER<br>
><br>
> You only need 2 prefixes, one for the keep and one for the NOT.<br>
> FileCheck also just got support for multiple prefixes in one run, so<br>
> you should be able to use just one FileCheck invocation with two<br>
> -check-prefix.<br>
<br>
</div>Heh… I’d missed the llvm-nm -no-sort option.  Fixed.<br>
<div><br>
> Nick, what do you think of the idea of using TargetLibraryInfo and<br>
> TargetLowering?<br>
<br>
</div>Nick?<br></blockquote><div><br></div><div>Sorry, lost context on this. Sounds good!</div><div><br></div><div>Nick</div><div> </div></div><br></div></div>