llvm dev - Mar 2013 - [LLVMdev] [cfe-dev] [RFC] TileGX, a new backend for Tilera's many core processor

On Mar 8, 2013, at 2:31 AM, Jiong Wang <jiwang at tilera.com> wrote:
> On 03/08/2013 04:48 AM, Dmitri Gribenko wrote:
>> On Thu, Mar 7, 2013 at 6:33 PM, Jiong Wang <jiwang at tilera.com>
wrote:
>>> Hi all,
>>> 
>>> Updated the patches for TILE-Gx backend:
>>> 
>>> 1. added initial regression tests for tilegx codegen.
>>> 2. added initial regression tests for MC Layer.
>>> 3. fixed those commenting style issues.
>>> 
>>> please review, thanks.
>> This is a huge patch, and reviewing it in tar.gz is hard.  To
>> facilitate review process, you can upload this to phabricator.
> 
>  OK, I am requesting for a phabricator account.
> 
>  one other things is,  should I split the patch into several parts? like
> 
>  [PATCH 1/10]
>  [PATCH 2/10]
>   ...
> 
>  because clang patch & test-suite patch is relatively small, but llvm
patch is nearly 10K line, still hard to review.
> 
>  if it is, will one patch for one file OK?
> 
>  What's the patch policy in llvm community ?
> 
Hi Jiong,

The LLVM community prefers small, self-contained patches. Please split up your
patches into small chunks that can be easily reviewed. Keep in mind that the
compiler needs to work after each pass goes in. :-)

-bw

On Fri, Mar 08, 2013 at 11:13:04AM -0800, Bill Wendling
wrote:
> The LLVM community prefers small, self-contained patches. Please split
> up your patches into small chunks that can be easily reviewed. Keep in
> mind that the compiler needs to work after each pass goes in. :-)
Just like with AArch64, this is not a reasonable requirement for a new
backend. The tarball contains essentially three different parts:
(1) Generic changes to recogniz TileGX as triple etc.
(2) The target subdirectory.
(3) The clang logic for va_arg etc.

While (1) can be and should be split off, it doesn't change match in
terms of patch size, since the majority will stay in item (2).

Joerg

On Mar 8, 2013, at 3:26 PM, Joerg Sonnenberger <joerg at
britannica.bec.de> wrote:
> On Fri, Mar 08, 2013 at 11:13:04AM -0800, Bill Wendling wrote:
>> The LLVM community prefers small, self-contained patches. Please split
>> up your patches into small chunks that can be easily reviewed. Keep in
>> mind that the compiler needs to work after each pass goes in. :-)
> 
> Just like with AArch64, this is not a reasonable requirement for a new
> backend. The tarball contains essentially three different parts:
> (1) Generic changes to recogniz TileGX as triple etc.
> (2) The target subdirectory.
> (3) The clang logic for va_arg etc.
> 
> While (1) can be and should be split off, it doesn't change match in
> terms of patch size, since the majority will stay in item (2).
> 
There are exceptions to every rule. :-) The upshot is that it needs to be fairly
easy to review.

-bw