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

[Dávid Bolvanský via llvm-commits]

Thanks, I will know for the next time that I can add you as the reviewer in
"InstCombine optimizations" which can do a proper review, or at least write
your opinion.

2018-05-18 0:13 GMT+02:00 Eric Christopher <echristo at gmail.com>:

>
>
> On Thu, May 17, 2018, 3:09 PM Friedman, Eli <efriedma at codeaurora.org>
> wrote:
>
>> 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.
>>
>
> If you're happy with it at this point I'm fine. Pending any more bugs from
> Ben :)
>
> Mostly needs to be clear that this needed an actual approval.
>
> -eric
>
>
> --
>> Employee of Qualcomm Innovation Center, Inc.
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180518/9e3da011/attachment.html>