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

Wed Apr 21 11:21:38 PDT 2021

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:

> >> 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.

While my point that i'm trying to make is: *try* to present the author
with said knowledge first.
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 :) )

> >> 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