<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div>I actually updated the patch after receiving Eric's comments on the function name and adding more comments.</div><div>> +// This function updates condition code for CMOV Opcodes.<br>> +// The input condition code can be E,NE,L,LE,G,GE,B,BE,A,AE and<br>> +// the updated condition code will be E,NE,G,GE,L,LE,A,AE,B,BE.<br>> +// This is to convert from a > b to b < a, a >= b to b <= a etc.<br>> +static unsigned UpdateCMovCondToOptimizeCmp(unsigned CMovOpc) {</div><div><br></div><div>But I didn't send out the updated patch before committing it.</div><div><br></div><div>As to the function and method names, the two functions already exist in ARM, I am simply reusing the names.</div><div>Same for the variable names & the loops.</div><div>I will try to follow the standard of "lower case for function name" and "upper case for variable name".</div><div><br></div><div>Yes, I am new to LLVM and the reviewing process.</div><div>Do I need to wait for at least one "LGTM" after starting the pre-commit review?</div><div><br></div>BTW, you can use "she" when referring to me :)<div><br></div><div>Manman</div><div><br><div><div><div>On Jun 4, 2012, at 12:46 PM, Chandler Carruth wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div class="gmail_quote">On Mon, Jun 4, 2012 at 12:26 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div class="im"><br><div><div>On Jun 2, 2012, at 3:55 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:</div><br><blockquote type="cite"><p><br>
On Fri, Jun 1, 2012 at 12:49 PM, Manman Ren <<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>> wrote:<br>
><br>
> Author: mren<br>
> Date: Fri Jun 1 14:49:33 2012<br>
> New Revision: 157831<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=157831&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=157831&view=rev</a><br>
> Log:<br>
> X86: peephole optimization to remove cmp instruction<br></p><p>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.</p>
</blockquote><br></div></div><div>I don't understand your frustration, Chandler.</div><div><br></div><div>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:</div>
<div><p style="font-family:Times;text-align:left">LLVM has a code review policy. Code review is one way to increase the quality of software. We generally follow these policies:</p><ol style="font-family:Times;text-align:left">
<li>All developers are required to have significant changes reviewed before they are committed to the repository.</li><li>Code reviews are conducted by email, usually on the llvm-commits list.</li><li>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.</li>
<li>The developer responsible for a code change is also responsible for making all necessary review-related changes.</li><li>Code review can be an iterative process, which continues until the patch is ready to be committed.</li>
</ol><div>This peephole optimization hardly seems like a "significant change" or a "major change", so according to our policy, it can be reviewed after commit.</div></div></div></blockquote><div><br></div>
<div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><div>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...</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>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.</div>
</div></blockquote><div><br></div><div>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.</div><div><br></div><div>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. ;]</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> (There was some question whether it might have been causing a regression, so reverting on that basis was a good idea.)</div>
</div></blockquote><div><br></div><div>Exactly. Regression or breakage -> revert. Otherwise, fix-forward. I think we're on the same page there.</div><div><br></div><div>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.</div>
</div>
</blockquote></div><br></div></div></body></html>