llvm dev - Mar 2013 - [LLVMdev] About commit TILE-Gx backend to community repository and default disabled

On Sat, Mar 23, 2013 at 04:59:49PM +0100, Tobias Grosser
wrote:
> With Chris's reply you also got the general OK to upstream the
> patch, _AFTER_ the actual patches have been reviewed.
One thing I was asking on IRC for is whether it makes sense for new
backends to be committable in incremental steps. Especially for a
complete backend like this, e.g. the asm parser would make a reasonable
and useful part to get in and limit the amount of code to be reviewed at
one point.

Joerg

于 2013/3/24 0:07, Joerg Sonnenberger 写道:
> On Sat, Mar 23, 2013 at 04:59:49PM +0100, Tobias Grosser wrote:
>> With Chris's reply you also got the general OK to upstream the
>> patch, _AFTER_ the actual patches have been reviewed.
> One thing I was asking on IRC for is whether it makes sense for new
> backends to be committable in incremental steps. Especially for a
> complete backend like this, e.g. the asm parser would make a reasonable
> and useful part to get in and limit the amount of code to be reviewed at
Hi Tobias & Joerg,

I understand your concern.

yes, Asm Parser is a module which could be committed individually, but I 
think the code
for AsmParser is relatively small and highly target dependent, a full 
regression test which
cover all instructions could largly gurantee the correctness.

I know contributors have their own works. For TILE-Gx backend, it's good 
if any contributor could have time to finish a full set review, I have 
been wating for several weeks.

I personly think it's not premature to commit TILE-Gx backend into 
mainline and default disable. Because:

1. TILE-Gx finished the regression test & test-suite test.
2. TILE-Gx buildbot will be setup, so it will be convinent for both 
community & Tilera company to monitor the status.

So should I keep waiting the full set review? or it's OK to commit and 
default disabled?

---
Regards,
Jiong
Tilera Corporation.
> one point.
>
> Joerg
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

On Sat, Mar 23, 2013 at 12:34 PM, Jiong Wang <jiwang at tilera.com> wrote:
> 于 2013/3/24 0:07, Joerg Sonnenberger 写道:
>
>  On Sat, Mar 23, 2013 at 04:59:49PM +0100, Tobias Grosser wrote:
>>
>>> With Chris's reply you also got the general OK to upstream the
>>> patch, _AFTER_ the actual patches have been reviewed.
>>>
>> One thing I was asking on IRC for is whether it makes sense for new
>> backends to be committable in incremental steps. Especially for a
>> complete backend like this, e.g. the asm parser would make a reasonable
>> and useful part to get in and limit the amount of code to be reviewed
at
>>
>
> Hi Tobias & Joerg,
>
> I understand your concern.
>
> yes, Asm Parser is a module which could be committed individually, but I
> think the code
> for AsmParser is relatively small and highly target dependent, a full
> regression test which
> cover all instructions could largly gurantee the correctness.
>
> I know contributors have their own works. For TILE-Gx backend, it's
good
> if any contributor could have time to finish a full set review, I have been
> wating for several weeks.
>
> I personly think it's not premature to commit TILE-Gx backend into
> mainline and default disable. Because:
>
> 1. TILE-Gx finished the regression test & test-suite test.
> 2. TILE-Gx buildbot will be setup, so it will be convinent for both
> community & Tilera company to monitor the status.
>
> So should I keep waiting the full set review? or it's OK to commit and
> default disabled?
>
>
The AArch64 backend was committed fairly quickly after being proposed for
inclusion, and they did not split up the patch and most of their backend
was committed in one huge commit.

Although IIRC Bill Wendling (CC'd) explicitly asked you to split the
backend into separate patches, it might be best to put it back together
again for the final review. AFAIK Phabricator does not have a good way to
keep all the patches somehow together, and otherwise they just get lost in
people's mail (and it would be annoying to ping 10+ patches). Having a
single patch avoids this problem and makes it a lot easier to keep track of
the backend. Phabricator has good support for commenting on specific parts
of the patch, so as long as the backend components are named in a
recognizable way (I think they are), experienced reviewers should not have
significant difficulty navigating (and they can always apply the patch
locally if necessary, which is easier with one huge patch).

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130323/275b3d4f/attachment.html>