[llvm-commits] [llvm] r157831 - in /llvm/trunk: lib/Target/X86/X86InstrArithmetic.td lib/Target/X86/X86InstrInfo.cpp lib/Target/X86/X86InstrInfo.h test/CodeGen/X86/jump_sign.ll

[Chandler Carruth]

On Mon, Jun 4, 2012 at 12:26 PM, Bob Wilson <bob.wilson at apple.com> wrote:

>
> On Jun 2, 2012, at 3:55 PM, Chandler Carruth <chandlerc at google.com> wrote:
>
>
> On Fri, Jun 1, 2012 at 12:49 PM, Manman Ren <mren at apple.com> wrote:
> >
> > Author: mren
> > Date: Fri Jun  1 14:49:33 2012
> > New Revision: 157831
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=157831&view=rev
> > Log:
> > X86: peephole optimization to remove cmp instruction
>
> I'm frustrated that you waited a total of 3 hours for review on the final
> version of this patch before committing it, w/o getting an LGTM. Eric even
> made comments on that thread, which he has had to repeat here. Please try
> to give the code review process a chance.
>
>
> I don't understand your frustration, Chandler.
>
> I'm not questioning your specific concerns about the patch, but you seem
> to have a different understanding of the process.  Here is what the LLVM
> Developer Policy says about code review:
>
> LLVM has a code review policy. Code review is one way to increase the
> quality of software. We generally follow these policies:
>
>    1. All developers are required to have significant changes reviewed
>    before they are committed to the repository.
>    2. Code reviews are conducted by email, usually on the llvm-commits
>    list.
>    3. Code can be reviewed either before it is committed or after. We
>    expect major changes to be reviewed before being committed, but smaller
>    changes (or changes where the developer owns the component) can be reviewed
>    after commit.
>    4. The developer responsible for a code change is also responsible for
>    making all necessary review-related changes.
>    5. Code review can be an iterative process, which continues until the
>    patch is ready to be committed.
>
> This peephole optimization hardly seems like a "significant change" or a
> "major change", so according to our policy, it can be reviewed after commit.
>

Ok, I see it as a significant change, but maybe that's inaccurate. The
things I don't see as significant are bug fixes, tuning performance,
refactoring following an already accepted design direction, etc.

Certainly this is fuzzy ground, but I think it's pretty clear to each
developer what constitues "significant" relative to their own historical
contributions. The first time I touched the x86 backend, I sent even bug
fix patches for pre-commit review because I didn't have the faintest clue
what I was doing. ;] They were all significant to me.

In this case, Manman mailed out the patch for pre-commit review, and so it
seemed to qualify for 'significant' compared to his past experience (which
afaict, is limited). Certainly the comments that have ensued have
reinforced this to my eyes...

In this case, I don't think it was necessary for Manman to revert the
> patch, at least not because of the code review comments.
>

I didn't ask for it to be reverted. I think revert is a strong thing, and I
like reserving that for when the patch is actively broken.

My frustration was that the pre-commit review was started, had not
concluded, and still had constructive feedback coming. Switching (without
notice) to post-commit review seems unwarranted and frustrating. That said,
I was trying mostly to give feedback for subsequent reviews. If I feel like
a patch need sto be reverted, I'll say so explicitly. ;]


>  (There was some question whether it might have been causing a regression,
> so reverting on that basis was a good idea.)
>

Exactly. Regression or breakage -> revert. Otherwise, fix-forward. I think
we're on the same page there.

My comment was just that I thought in the future, pre-commit review should
probably have continued a few more rounds, and was trying to help Manman
become better calibrated to the review process within the community.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120604/771b73cc/attachment.html>