[llvm] de18fa9 - Revert "[InstSimplify] Bypass no-op `and`-mask, using known bits (PR49543)"

Fri Apr 23 09:08:59 PDT 2021

On 4/21/21 11:21 AM, Roman Lebedev wrote:
> On Wed, Apr 21, 2021 at 9:08 PM Philip Reames <listmail at philipreames.com> wrote:
>>
>> On 4/21/21 10:29 AM, Roman Lebedev wrote:
>>> On Wed, Apr 21, 2021 at 7:54 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>>>> On Wed, Apr 21, 2021 at 6:21 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>>>>> 1. I don't recall getting a message "hey, this is causing xyz, any
>>>>> thoughts, should revert?"
>>>>> 2. I don't recall seeing a follow-up mail to the original commit that
>>>>> it was reverted
>>>> You replied to the revert email before I had a chance to message the original commit. From your reply to the revert I assumed you were already aware of the revert. Of course, now that you mention it, mailing the original commit may still be useful for others.
>>> Note that i specifically said this wasn't the only instance.
>>> Other examples:
>>> https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20210412/thread.html#903512
>>> https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20210315/thread.html#894082
>>> https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20210315/thread.html#893258
>>> <...>
>>>
>>> So these two points stand i think.
>>>
>>>> As you are a very heavy user of post-commit review, please expect that reverts of your commits will be more common than if you go through pre-commit review.
>>> I think this may be not spelled out enough. More common //why//?
>>> Because that post-commit review approach potentially results in more
>>> commits? Absolutely.
>>>
>>> Please correct if i'm wrong, but the docs only mention that the
>>> feedback post-commit is no less important than one pre-commit.
>>> But i don't recall seeing anything that says pre-reviewed code/commits
>>> is weaker/stronger for the purpose of post-commit review/reverts.
>> Roman, I think you're missing the point Nikita is making.  Let me try to
>> restate it.
>>
>> Changes that are reviewed pre-commit will tend to have issues resolved
>> during review which would have triggered a revert if landed.  They may
>> also not be approved at all.  As a result, you would expect changes
>> which have gone through review to be reverted less often.
>>
>> Does that rewording make sense?
>>
>> I'll also note that we are explicitly "pre-commit review with
>> exceptions", not "post-commit review with exceptions" as our default
>> policy.  You are expected to have changes pre-reviewed unless you are
>> confident that review is not needed.  This is a judgement call you are
>> expected to make.  Part of that trust is responding appropriately to
>> post-commit review when it is raised.
> All of this sounds good to me.
>
>>>> If you had gone through review with this change, I would have left my concerns on the review. As you did not go through a review, I reverted to status quo, indicated my primary point of contention, and suggested that the patch go through review.
>>>> In most cases I would simply leave comments on specific parts of the patch, with the expectation that they be addressed in due time. In this instance, I disagree with the patch in concept, so that wouldn't work.
>>> Why wouldn't it work? I think this may be the main problem here.
>>> And i disagree that llvm supports windows/provides clang-cl. Shall i
>>> go ahead and revert their commits? Concretely, this comment suggests
>>> that it wouldn't be addressable in a proper review, and it would be
>>> blocked with no hope for forward progress. But is that really so?
>> Roman, please stop and adjust your tone here.  IMO, you have crossed the
>> line of professional discourse and are being inappropriate hostile and
>> rude.  I would suggest you step back, take some time to calm down, and
>> come back to this with fresh eyes.
> Ugh. Actually, i think *my* point isn't being understood here.
> I guess this is due to my writing style. Let me dumb it down.
> I read:
Before anything else, a point on wording.  "Let me try to rephrase my 
point" or something analogous would have been much more appropriate here.
>
>>>> If you had gone through review with this change, I would have left my concerns on the review. As you did not go through a review, I reverted to status quo, indicated my primary point of contention, and suggested that the patch go through review.
>>>> In most cases I would simply leave comments on specific parts of the patch, with the expectation that they be addressed in due time. In this instance, I disagree with the patch in concept, so that wouldn't work.
> as "welp, this is fundamentally broken, let's override and back it out
> ourselves",
> with implicit subtext that the patch's author won't agree with that
> judgement call,
> and wouldn't have done so when presented with said knowledge,
> without actually presenting the author with said knowledge.
> All of this may or may not imply that author is assumed to be acting
> in bad faith.

I don't think anyone has acted in bad faith here, and I don't see any 
implication of such in any of the emails to date.  I think you're also 
misreading the situation a bit.  You seem to be focusing on the revert, 
not the objection.

The objection which was raised - and explained in both the revert 
message  - is that we have chosen not to use known bits in instsimplify 
as a means to control compile time.  (Instcombine covers it instead.) 
You should be able to find the previous discussions with some quick 
googling, but if not, let me know and I'll see if I can dig up some 
references for you.

Given this objection, if it had been raised as post commit review, I 
would expect you to revert the change.  Let me be very clear that a 
design objection is explicitly a reason for you to revert your own 
change.  Given that, Nikita's revert was him doing you a courtesy - i.e. 
saving you work.

It is worth noting explicitly that this is one of the aspects of llvm 
dev culture which is weird by other communities standards.  I recently 
made an attempt to document this in the developer policy specifically 
because of how surprising some people find it.

>
> While my point that i'm trying to make is: *try* to present the author
> with said knowledge first.
This is not our community norm.  It's not an unreasonable position to 
take, but it is not the way we handle reverts.
> Something tells me, most would act accordingly, and others would be
> good examples
> for the upcoming documentation change (it's upcoming, right?)
> (read: i would have done so, the fix is pretty obvious here :) )
I look forward to seeing your - instcombine presumably? - change.  I 
explicitly request you go through precommit review for this one w/myself 
and Nikita as reviewers (+ any other relevant parties).
>
>>>> Possibly I misunderstood our review/revert policy, but I was under the impression that it's always possible to request that a post-commit review be converted into a pre-commit review, with the change being reverted in the meantime.
>>>> Regards,
>>>> Nikita
>>> Roman
>>>
>>> On Wed, Apr 21, 2021 at 7:54 PM Nikita Popov <nikita.ppv at gmail.com> wrote:
>>>> On Wed, Apr 21, 2021 at 6:21 PM Roman Lebedev <lebedev.ri at gmail.com> wrote:
>>>>> 1. I don't recall getting a message "hey, this is causing xyz, any
>>>>> thoughts, should revert?"
>>>>> 2. I don't recall seeing a follow-up mail to the original commit that
>>>>> it was reverted
>>>> You replied to the revert email before I had a chance to message the original commit. From your reply to the revert I assumed you were already aware of the revert. Of course, now that you mention it, mailing the original commit may still be useful for others.
>>>>
>>>> As you are a very heavy user of post-commit review, please expect that reverts of your commits will be more common than if you go through pre-commit review. If you had gone through review with this change, I would have left my concerns on the review. As you did not go through a review, I reverted to status quo, indicated my primary point of contention, and suggested that the patch go through review.
>>>>
>>>> In most cases I would simply leave comments on specific parts of the patch, with the expectation that they be addressed in due time. In this instance, I disagree with the patch in concept, so that wouldn't work.
>>>>
>>>> Possibly I misunderstood our review/revert policy, but I was under the impression that it's always possible to request that a post-commit review be converted into a pre-commit review, with the change being reverted in the meantime.
>>>>
>>>> Regards,
>>>> Nikita