<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 2, 2015 at 5:17 PM, Chih-hung Hsieh <span dir="ltr"><<a href="mailto:chh@google.com" target="_blank">chh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Wed, Dec 2, 2015 at 3:47 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Wed, Dec 2, 2015 at 3:44 PM, Chih-Hung Hsieh <<a href="mailto:chh@google.com" target="_blank">chh@google.com</a>> wrote:<br>
> chh added a comment.<br>
><br>
> Sean, only the "Fix printOperand to handle null operand" in SelectionDAGDumper.cpp might have some value by itself.<br>
<br>
</span>If you can submit the smaller patches independently, it is still<br>
better to submit them separately -- it reduces the chances of build<br>
breaks.<br>
<span><font color="#888888"><br></font></span></blockquote><div><br></div></span><div><div>One small change is less risky than one large change, but</div><div>it's unclear to me that the total number of broken builds (downtime)</div><div>can be reduced by breaking a 10-file patch into 10 1-file patches.</div><div><br></div><div>If there are K errors, they will break builds K times</div><div>if we discover and fix them one by one.</div><div>The hope is to find error before submitting file #2 from the</div><div>regression of submitted file #1. But it seems more likely</div><div>to discover both errors in #1 and #2 when they are submitted together</div><div>and then reverted and fixed together.</div></div><span class="HOEnZb"><font color="#888888"><div><br></div></font></span></div></div></div></blockquote><div><br></div><div>It is a common wisdom to break patch into smaller pieces -- I will try my best to explain:</div><div>1) you don't get all errors at the same time when you submit a large patch -- some errors may be masked by previous errors</div><div>2) The more errors you incur at the same time, the bigger the chances other submitter's errors leaked in without being caught -- making it hard to isolate the problems later.</div><div>3) Errors in smaller patches are easier to correct -- in many cases you can fix the problem quicker than rolling it back</div><div>4) Smaller patches give other people a chance to peek at the patch and spot obvious errors missed in the review</div><div><br></div><div>Not to mention that smaller patch make it easier for downstream LLVM users to do merge more easiliy.</div><div><br></div><div>Breaking patch into smaller one, you do need to pay a little more cost up-front, but it is worth it.</div><div><br></div><div>(I am not saying you have to break it up in really small granularity -- you need to use some good judgement here).</div><div><br></div><div>David</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="HOEnZb"><font color="#888888"><div></div><div>-- chh</div></font></span><span class=""><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><font color="#888888">
David<br>
</font></span><div><div><br>
<br>
> All other changes are not useful before x86_64 f128 type is configured to fix the calling convention problem.<br>
> This part 1 change should have no impact at all before the rest of <a href="http://reviews.llvm.org/D11438" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11438</a> is merged in.<br>
> Getting this part 1 in first is just to check for any regression.<br>
> I think all these changes should go in or be reverted when necessary.<br>
> It would be easier to keep track of these changes together, right?<br>
<br>
<br>
<br>
<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D15134" rel="noreferrer" target="_blank">http://reviews.llvm.org/D15134</a><br>
><br>
><br>
><br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>