[PATCH] D15134: Part 1 to fix x86_64 fp128 calling convention.

[Chih-hung Hsieh via llvm-commits]

David, I think you have a pretty good picture of the complete patch
in D11438 now. It seems premature of me to split that patch before I have
made all your suggested changes. Now I have to test and upload every change
with both D11438 and D15134.

Could we complete all code review and changes before further split?
Thanks.


On Wed, Dec 2, 2015 at 6:24 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Wed, Dec 2, 2015 at 5:17 PM, Chih-hung Hsieh <chh at google.com> wrote:
>
>> On Wed, Dec 2, 2015 at 3:47 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>> On Wed, Dec 2, 2015 at 3:44 PM, Chih-Hung Hsieh <chh at google.com> wrote:
>>> > chh added a comment.
>>> >
>>> > Sean, only the "Fix printOperand to handle null operand" in
>>> SelectionDAGDumper.cpp might have some value by itself.
>>>
>>> If you can submit the smaller patches independently, it is still
>>> better to submit them separately  -- it reduces the chances of build
>>> breaks.
>>>
>>>
>> One small change is less risky than one large change, but
>> it's unclear to me that the total number of broken builds (downtime)
>> can be reduced by breaking a 10-file patch into 10 1-file patches.
>>
>> If there are K errors, they will break builds K times
>> if we discover and fix them one by one.
>> The hope is to find error before submitting file #2 from the
>> regression of submitted file #1. But it seems more likely
>> to discover both errors in #1 and #2 when they are submitted together
>> and then reverted and fixed together.
>>
>>
> It is a common wisdom to break patch into smaller pieces -- I will try my
> best to explain:
> 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
> 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.
> 3) Errors in smaller patches are easier to correct -- in many cases you
> can fix the problem quicker than rolling it back
> 4) Smaller patches give other people a chance to peek at the patch and
> spot obvious errors missed in the review
>
> Not to mention that smaller patch make it easier for downstream LLVM users
> to do merge more easiliy.
>
> Breaking patch into smaller one, you do need to pay a little more cost
> up-front, but it is worth it.
>
> (I am not saying you have to break it up in really small granularity --
> you need to use some good judgement here).
>
> David
>
>
>
>
>> -- chh
>>
>>
>>
>>
>>> David
>>>
>>>
>>> > All other changes are not useful before x86_64 f128 type is configured
>>> to fix the calling convention problem.
>>> > This part 1 change should have no impact at all before the rest of
>>> http://reviews.llvm.org/D11438 is merged in.
>>> > Getting this part 1 in first is just to check for any regression.
>>> > I think all these changes should go in or be reverted when necessary.
>>> > It would be easier to keep track of these changes together, right?
>>>
>>>
>>>
>>>
>>> >
>>> >
>>> > http://reviews.llvm.org/D15134
>>> >
>>> >
>>> >
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151203/fbf67bcf/attachment.html>