[LLVMdev] About commit TILE-Gx backend to community repository and default disabled

[Sean Silva]

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/cfe-commits/attachments/20130323/275b3d4f/attachment.html>