[llvm] r332452 - [SimplifyLibcalls] Replace locked IO with unlocked IO

[Eric Christopher via llvm-commits]

I'm currently not entirely sure we want to do this class of optimizations
for a few reasons:

a) This ends up being an interesting educational problem on the contents of
a particular platform's libc - in particular, people that write small libc
interceptors are going to have to update just for this in order to
successfully build. I updated a few recently, but this seems to be
something that's going to be frustrating at llvm release time. It will
require anyone that's using a libc interceptor to run into some odd
problems if they don't -fno-builtin some new functions or make sure that
they add the unlocked versions which are technically not required, but may
exist on a system.

b) I believe I've found a bug in the current (theoretical) basis of this
which is: fopen could capture the file argument and do whatever it wants
with it. (and even after my fix, -fno-builtin-fopen still didn't have
individual -fno-builtin support which means you really couldn't distinguish
this at the time from clang - I've since fixed this). This may not be the
case anymore, but...

What's the overall gain we're getting for doing this optimization? It seems
to be a lot of pain for not a lot of win in general applications? Is there
some benchmark here?

Thoughts?

-eric

On Thu, May 17, 2018 at 3:13 PM Dávid Bolvanský via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> You are right so I would like to apologize you.
>
> If you revert it, I would be fine too. I do my best to do/fix things in
> this patch anyway :)
>
> Dňa pi 18. 5. 2018, 0:09 Friedman, Eli <efriedma at codeaurora.org>
> napísal(a):
>
>> On 5/17/2018 2:51 PM, Eric Christopher wrote:
>>
>>
>>
>> On Wed, May 16, 2018 at 3:09 PM Friedman, Eli via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> On 5/16/2018 2:54 PM, Benjamin Kramer via llvm-commits wrote:
>>> > This triggered a really annoying miscompile (see r332531), and it's
>>> even
>>> > present in the test case. Please double check your tests, especially
>>> when
>>> > using update_test_checks.py.
>>> >
>>> > Also you want to get these things reviewed by someone who's actually
>>> > familiar with the LLVM before they land.
>>>
>>> I looked at the patch multiple times, and never spotted this issue.
>>> Granted, maybe I should have looked a little more carefully at that part
>>> of the patch (I was more focused on the actual transform).
>>>
>>
>> Probably would have helped here if you'd ack'd the patch yourself. The
>> only approval on the patch is an anonymous person with no llvm history (as
>> far as I can tell).
>>
>> -eric
>>
>> The timeline was something like this: I reviewed it on May 8, and gave it
>> sort of tentative LGTM, pending the discussion on llvmdev.  The discussion
>> happened on llvmdev, David posted a slightly updated path in response to
>> that discussion, and I meant to review that, but it sort of sank to the
>> bottom of my inbox, and David merged on the 15th (?) without a review on
>> the updated version.  (I really shouldn't have let it sit that long, but
>> you know how reviews tend to pile up.)
>>
>> David shouldn't have merged it without an explicit ack from me or another
>> qualified reviewer, but I don't think we need to revert at this point.
>>
>> -Eli
>>
>> --
>> Employee of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
>> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180531/6447bc02/attachment.html>