Joerg Sonnenberger
2013-Mar-23 16:07 UTC
[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
Jiong Wang
2013-Mar-23 16:34 UTC
[LLVMdev] About commit TILE-Gx backend to community repository and default disabled
于 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 atHi 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
Sean Silva
2013-Mar-24 02:05 UTC
[LLVMdev] About commit TILE-Gx backend to community repository and default disabled
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>
Reasonably Related Threads
- [LLVMdev] About commit TILE-Gx backend to community repository and default disabled
- [LLVMdev] About commit TILE-Gx backend to community repository and default disabled
- [LLVMdev] About commit TILE-Gx backend to community repository and default disabled
- [LLVMdev] About commit TILE-Gx backend to community repository and default disabled
- [LLVMdev] About commit TILE-Gx backend to community repository and default disabled