<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">Was this ever fully resolved?</div><div class=""><br class=""></div><div class="">Folks on this thread were interested in finding out more about the cost/benefit tradeoff of doing this optimization. I'm not sure that's been adequately explored.</div><div class=""><br class=""></div><br class=""><blockquote type="cite" class="">On May 31, 2018, at 12:34 PM, Dávid Bolvanský via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class=""></blockquote><div><blockquote type="cite" class=""><br class="Apple-interchange-newline"><div class=""><div dir="auto" class=""><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:"segoe ui","segoe ui emoji","segoe ui symbol",lato,"helvetica neue",helvetica,arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)" class="">In my small C tests and with some file io-related open source programs I see some small improvements - yes, nothing "wow" - but still worth for me to try to do it in LLVM.</p></div></div></blockquote>I'd like to see a fuller justification here. Could you elaborate?</div><div><br class=""></div><div>- What does the typical improvement look like?</div><div><br class=""></div><div>- How common is this pattern is in the open source programs you looked at?</div><div><br class=""></div><div>- What do you think the policy should be for simplifying libcalls in a way that can break interceptors?<br class=""><div><br class=""></div><div>Could you CC the reviewer, rja (Joe) on this thread? They may have some insight. I can't find their email address. They appear to have stopped commenting on Phab shortly after this commit landed.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="auto" class=""><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:"segoe ui","segoe ui emoji","segoe ui symbol",lato,"helvetica neue",helvetica,arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)" class="">I am not sure about the possible rule "if no interesting numbers in SPEC, do not do it". I still consider this as a quite interesting transformation. And I learnt a lot during this work.</p></div></div></blockquote><div>It's great that you find this work interesting and educational, but that alone isn't a sufficient justification for adding an optimization.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="auto" class=""><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:"segoe ui","segoe ui emoji","segoe ui symbol",lato,"helvetica neue",helvetica,arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)" class="">We could remove the whole SimplifyLibcalls since it brings micro optimizations. Sorry to say, but then should be a warning: don't work on small/micro optimizations, we dont like/want it.</p></div></div></blockquote><div>There may be a high bar for clearing certain changes, but I'd like to think that the llvm community is generally welcoming :). Equating concern over one change to a removal of a chunk of the compiler is a false comparison.</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div dir="auto" class=""><p style="margin:0px 0px 12px;padding:0px;border:0px;font-family:"segoe ui","segoe ui emoji","segoe ui symbol",lato,"helvetica neue",helvetica,arial,sans-serif;font-size:13px;background-color:rgb(255,255,255)" class="">If you have list of things which are "todo/we would like to focus on them" for LLVM, I would like to see them. So maybe I could work on something less controversal (vs. this patch) :)</p></div></div></blockquote>Some of the projects here are actively being worked on for GSoC, but I'm sure any number of them could use more help: <a href="http://llvm.org/OpenProjects.html" class="">http://llvm.org/OpenProjects.html</a>.</div><div><br class=""></div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div dir="auto" class=""><div style="margin: 0px; padding: 0px; border: 0px; font-family: "segoe ui", "segoe ui emoji", "segoe ui symbol", lato, "helvetica neue", helvetica, arial, sans-serif; font-size: 13px; background-color: rgb(255, 255, 255);" class="">tl;dr: I am fine if a there should be a new option to enable it (default: off) or if you decide to fully remove it.</div></div></div></blockquote><div><br class=""></div><div>I think it would be fine to keep this change in tree provided that we get a better understanding of the tradeoffs here.</div><div><br class=""></div><div>thanks!</div><div>vedant</div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">Dňa št 31. 5. 2018, 21:00 Friedman, Eli <<a href="mailto:efriedma@codeaurora.org" class="">efriedma@codeaurora.org</a>> napísal(a):<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 5/31/2018 12:07 AM, Eric Christopher wrote:<br class="">
> I'm currently not entirely sure we want to do this class of <br class="">
> optimizations for a few reasons:<br class="">
><br class="">
> a) This ends up being an interesting educational problem on the <br class="">
> contents of a particular platform's libc - in particular, people that <br class="">
> write small libc interceptors are going to have to update just for <br class="">
> this in order to successfully build. I updated a few recently, but <br class="">
> this seems to be something that's going to be frustrating at llvm <br class="">
> release time. It will require anyone that's using a libc interceptor <br class="">
> to run into some odd problems if they don't -fno-builtin some new <br class="">
> functions or make sure that they add the unlocked versions which are <br class="">
> technically not required, but may exist on a system.<br class="">
<br class="">
Yes, this is a concern. But I'm not sure where exactly we would draw <br class="">
the line here; do we want to document some "minimal" set of C library <br class="">
functions that LLVM might call for each target, given input which <br class="">
doesn't explicitly call those functions? Or are functions that do I/O <br class="">
special? Or do we just want to use some informal "don't rock the boat <br class="">
unless we see improvements on SPEC" rule? (We already do various <br class="">
similar transforms, like printf->puts, sin->sinf, automatic recognition <br class="">
of memset_pattern16, etc.)<br class="">
<br class="">
> b) I believe I've found a bug in the current (theoretical) basis of <br class="">
> this which is: fopen could capture the file argument and do whatever <br class="">
> it wants with it. (and even after my fix, -fno-builtin-fopen still <br class="">
> didn't have individual -fno-builtin support which means you really <br class="">
> couldn't distinguish this at the time from clang - I've since fixed <br class="">
> this). This may not be the case anymore, but...<br class="">
<br class="">
Assuming fopen returns a unique FILE* is just like assuming malloc <br class="">
returns a unique memory allocation; it's just assuming the C library <br class="">
works as documented. This doesn't seem particularly controversial.<br class="">
<br class="">
> What's the overall gain we're getting for doing this optimization? It <br class="">
> seems to be a lot of pain for not a lot of win in general <br class="">
> applications? Is there some benchmark here?<br class="">
<br class="">
Dávid said he saw substantial gains on microbenchmarks... not sure what <br class="">
sort of general benchmarking was involved. I'll let him comment on that <br class="">
more.<br class="">
<br class="">
-Eli<br class="">
<br class="">
-- <br class="">
Employee of Qualcomm Innovation Center, Inc.<br class="">
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project<br class="">
<br class="">
</blockquote></div>
_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></body></html>