llvm dev - Nov 2012 - [LLVMdev] [llvm-commits] RFC: Merge branches/R600 into TOT for 3.2 release

On Sat, Nov 17, 2012 at 10:56:26PM +0100, Benjamin Kramer
wrote:
> 
> On 01.11.2012, at 14:44, Tom Stellard <tom at stellard.net> wrote:
> 
> > Moving this thread to llvmdev.
> > 
> > On Tue, Oct 30, 2012 at 11:09:34PM -0700, Chris Lattner wrote:
> >> On Oct 30, 2012, at 11:35 AM, Tom Stellard <tom at
stellard.net> wrote:
> >>>> Hi Tom,
> >>>> 
> >>>> Time is running short, but this would be great.  The best
place to start is to begin decomposing the mega-patch into individual pieces
that makes sense.  Do you have changes outside Target/AMDGPU?  Those would be
the be place to start.
> >>> 
> >>> Not counting test cases, new intrinsics, and configuration
files,
> >>> there are no changes outside of Target/AMDGPU.  I always
submit patches
> >>> for changes I make outside of Target/AMDGPU.  Isn't that
how Open Source
> >>> is supposed to work? ;)
> >> 
> >> Well yeah, but sometimes you still have to ask :).  Can the
intrinsics be pulled into the target directory ala
Targets/MBlaze/MBlazeIntrinsics.td or Targets/NVPTX/NVPTXIntrinsics.td?
> >> 
> >>> What's the best way to break up the patch?  In the past I
have divided it up
> >>> into 4 patches:
> >>> 
> >>> 1. Core Target files (*TargetMachine, *InstInfo, etc.)
> >>> 2. Target specific passes
> >>> 3. .td files
> >>> 4. Configuration / Intrinsic additions
> >>> 
> >>> Would something like this work?
> >> 
> >> Any way that you can slices it to land it in stages would be
preferred.  Another major challenge will be to find someone to review the patch.
You don't need detailed comments on every design aspect, but it is important
to give you a basic "yeah, the structure looks fine and there aren't
any obviously gratuitous code style violations" is the sort of review
I'm looking for.  I'm not sure a great way to get this, but asking on
llvmdev is probably a good start.
> >> 
> > 
> > Hi LLVM Developers,
> > 
> > I would like to merge the R600 backend from branches/R600 into TOT
> > prior to the LLVM 3.2 release.  As you can see in the above paragraph,
> > Chris has asked me to find someone to review the code.  If there is
> > anyone that would be willing to take the time and look at the code,
> > I would really appreciate it.  For your convenience, I have split the
> > code into two patches:
> > 
> > http://people.freedesktop.org/~tstellar/llvm/R600.patch
> > http://people.freedesktop.org/~tstellar/llvm/SI.patch
> > 
> > R600.patch adds the code required to support the R600 generation of
GPUs,
> > and SI.patch adds the code required to support the Southern Islands
> > generation of GPUs.
> > 
> > In addition, this code can be found in the branches/R600 branch of the
> > llvm SVN repository, as well as in my git repo:
> > 
> > git fetch git://people.freedesktop.org/~tstellar/llvm master
> > gitweb: http://cgit.freedesktop.org/~tstellar/llvm/
> > 
> > In the attached patches, the R600 backend is not built by default,
> > so you need to pass the --enable-experimental-targets=AMDGPU to build
> > it.  If the backend is merged into TOT, I was planning to keep the
> > build disabled by default, but I don't mind enabling it if someone
wants
> > to approve that change.
> > 
> > I will also be changing the name of the backend directory from AMDGPU
to
> > R600 per Chris' suggestion.  These patches still use the AMDGPU
> > directory, but I'll be updating the R600 branch with this change
> > as soon as I am able to pass along updated build instructions to our
users.
> > 
> > Looking forward to your comments.
> 
Hi Ben,

Thank you for reviewing the code!
> I went over the patches today and I must say that it looks a lot better
than when you started getting it in shape for inclusion in LLVM. Great Job!
> 
> While I can't comment on technical details of the architecture, here
are my high level style comments:
> 
> - The prevailing style of brace placement in LLVM is to never put the
opening brace on it's own line. There are different styles used in your
patches.
> - In some places the indentation uses 4 or more spaces instead of the
preferred two.
> - There's commented out code in both c++ source and the .td files.
Please either document why it's commented out or remove the code.
"//FIXME: gcc complains on this." isn't helpful at all.
> - Macros like
> #define FirstNonDebugInstr(A) A->begin()
> are not only confusing, the name also says something that the definition
doesn't provide. Please audit the macros and change them to UPPERCASE so
it's obvious that there's some macro magic at work.
> 
I've made these changes and also renamed the directory from AMDGPU
to R600 as suggested by Chris.  An updated patch can be found here:
http://people.freedesktop.org/~tstellar/llvm/Nov26/

I've also included in that directory the 6 patches I wrote to implement
these changes, to make it easier to see what is different.  If you see
anything I left out, let me know.
> Overall I think this should go in as soon as possible. It has been around
for a while and you have been actively maintaining it over the whole timespan
since it was first proposed for inclusion. I'm afraid it's too late for
3.2, but we're now early in the 3.3 cycle and it would be really nice to
have a stable R600 backend in 3.3.
> 
Assuming I caught all the coding style mistakes you pointed out, what
else needs to be done to be able to commit this code?

Thanks,
Tom

On 26.11.2012, at 23:37, Tom Stellard <tom at stellard.net> wrote:
> On Sat, Nov 17, 2012 at 10:56:26PM +0100, Benjamin Kramer wrote:
>> 
>> On 01.11.2012, at 14:44, Tom Stellard <tom at stellard.net>
wrote:
>> 
>>> Moving this thread to llvmdev.
>>> 
>>> On Tue, Oct 30, 2012 at 11:09:34PM -0700, Chris Lattner wrote:
>>>> On Oct 30, 2012, at 11:35 AM, Tom Stellard <tom at
stellard.net> wrote:
>>>>>> Hi Tom,
>>>>>> 
>>>>>> Time is running short, but this would be great.  The
best place to start is to begin decomposing the mega-patch into individual
pieces that makes sense.  Do you have changes outside Target/AMDGPU?  Those
would be the be place to start.
>>>>> 
>>>>> Not counting test cases, new intrinsics, and configuration
files,
>>>>> there are no changes outside of Target/AMDGPU.  I always
submit patches
>>>>> for changes I make outside of Target/AMDGPU.  Isn't
that how Open Source
>>>>> is supposed to work? ;)
>>>> 
>>>> Well yeah, but sometimes you still have to ask :).  Can the
intrinsics be pulled into the target directory ala
Targets/MBlaze/MBlazeIntrinsics.td or Targets/NVPTX/NVPTXIntrinsics.td?
>>>> 
>>>>> What's the best way to break up the patch?  In the past
I have divided it up
>>>>> into 4 patches:
>>>>> 
>>>>> 1. Core Target files (*TargetMachine, *InstInfo, etc.)
>>>>> 2. Target specific passes
>>>>> 3. .td files
>>>>> 4. Configuration / Intrinsic additions
>>>>> 
>>>>> Would something like this work?
>>>> 
>>>> Any way that you can slices it to land it in stages would be
preferred.  Another major challenge will be to find someone to review the patch.
You don't need detailed comments on every design aspect, but it is important
to give you a basic "yeah, the structure looks fine and there aren't
any obviously gratuitous code style violations" is the sort of review
I'm looking for.  I'm not sure a great way to get this, but asking on
llvmdev is probably a good start.
>>>> 
>>> 
>>> Hi LLVM Developers,
>>> 
>>> I would like to merge the R600 backend from branches/R600 into TOT
>>> prior to the LLVM 3.2 release.  As you can see in the above
paragraph,
>>> Chris has asked me to find someone to review the code.  If there is
>>> anyone that would be willing to take the time and look at the code,
>>> I would really appreciate it.  For your convenience, I have split
the
>>> code into two patches:
>>> 
>>> http://people.freedesktop.org/~tstellar/llvm/R600.patch
>>> http://people.freedesktop.org/~tstellar/llvm/SI.patch
>>> 
>>> R600.patch adds the code required to support the R600 generation of
GPUs,
>>> and SI.patch adds the code required to support the Southern Islands
>>> generation of GPUs.
>>> 
>>> In addition, this code can be found in the branches/R600 branch of
the
>>> llvm SVN repository, as well as in my git repo:
>>> 
>>> git fetch git://people.freedesktop.org/~tstellar/llvm master
>>> gitweb: http://cgit.freedesktop.org/~tstellar/llvm/
>>> 
>>> In the attached patches, the R600 backend is not built by default,
>>> so you need to pass the --enable-experimental-targets=AMDGPU to
build
>>> it.  If the backend is merged into TOT, I was planning to keep the
>>> build disabled by default, but I don't mind enabling it if
someone wants
>>> to approve that change.
>>> 
>>> I will also be changing the name of the backend directory from
AMDGPU to
>>> R600 per Chris' suggestion.  These patches still use the AMDGPU
>>> directory, but I'll be updating the R600 branch with this
change
>>> as soon as I am able to pass along updated build instructions to
our users.
>>> 
>>> Looking forward to your comments.
>> 
> 
> Hi Ben,
> 
> Thank you for reviewing the code!
> 
>> I went over the patches today and I must say that it looks a lot better
than when you started getting it in shape for inclusion in LLVM. Great Job!
>> 
>> While I can't comment on technical details of the architecture,
here are my high level style comments:
>> 
>> - The prevailing style of brace placement in LLVM is to never put the
opening brace on it's own line. There are different styles used in your
patches.
>> - In some places the indentation uses 4 or more spaces instead of the
preferred two.
>> - There's commented out code in both c++ source and the .td files.
Please either document why it's commented out or remove the code.
"//FIXME: gcc complains on this." isn't helpful at all.
>> - Macros like
>> #define FirstNonDebugInstr(A) A->begin()
>> are not only confusing, the name also says something that the
definition doesn't provide. Please audit the macros and change them to
UPPERCASE so it's obvious that there's some macro magic at work.
>> 
> 
> I've made these changes and also renamed the directory from AMDGPU
> to R600 as suggested by Chris.  An updated patch can be found here:
> http://people.freedesktop.org/~tstellar/llvm/Nov26/
Sorry for being pedantic, but there are still misplaced braces in your patch.
You can easily find them in the source code with:

$ grep -r '^[ ]*{$'

Otherwise I don't see anything else that needs fixing offhand.
> I've also included in that directory the 6 patches I wrote to implement
> these changes, to make it easier to see what is different.  If you see
> anything I left out, let me know.
> 
>> Overall I think this should go in as soon as possible. It has been
around for a while and you have been actively maintaining it over the whole
timespan since it was first proposed for inclusion. I'm afraid it's too
late for 3.2, but we're now early in the 3.3 cycle and it would be really
nice to have a stable R600 backend in 3.3.
>> 
> 
> Assuming I caught all the coding style mistakes you pointed out, what
> else needs to be done to be able to commit this code?
IMO this can go in now, but you'll need approval from Chris.

- Ben

On 26.11.2012, at 23:37, Tom Stellard <tom at stellard.net> wrote:
> On Sat, Nov 17, 2012 at 10:56:26PM +0100, Benjamin Kramer wrote:
>> 
>> On 01.11.2012, at 14:44, Tom Stellard <tom at stellard.net>
wrote:
>> 
>>> Moving this thread to llvmdev.
>>> 
>>> On Tue, Oct 30, 2012 at 11:09:34PM -0700, Chris Lattner wrote:
>>>> On Oct 30, 2012, at 11:35 AM, Tom Stellard <tom at
stellard.net> wrote:
>>>>>> Hi Tom,
>>>>>> 
>>>>>> Time is running short, but this would be great.  The
best place to start is to begin decomposing the mega-patch into individual
pieces that makes sense.  Do you have changes outside Target/AMDGPU?  Those
would be the be place to start.
>>>>> 
>>>>> Not counting test cases, new intrinsics, and configuration
files,
>>>>> there are no changes outside of Target/AMDGPU.  I always
submit patches
>>>>> for changes I make outside of Target/AMDGPU.  Isn't
that how Open Source
>>>>> is supposed to work? ;)
>>>> 
>>>> Well yeah, but sometimes you still have to ask :).  Can the
intrinsics be pulled into the target directory ala
Targets/MBlaze/MBlazeIntrinsics.td or Targets/NVPTX/NVPTXIntrinsics.td?
>>>> 
>>>>> What's the best way to break up the patch?  In the past
I have divided it up
>>>>> into 4 patches:
>>>>> 
>>>>> 1. Core Target files (*TargetMachine, *InstInfo, etc.)
>>>>> 2. Target specific passes
>>>>> 3. .td files
>>>>> 4. Configuration / Intrinsic additions
>>>>> 
>>>>> Would something like this work?
>>>> 
>>>> Any way that you can slices it to land it in stages would be
preferred.  Another major challenge will be to find someone to review the patch.
You don't need detailed comments on every design aspect, but it is important
to give you a basic "yeah, the structure looks fine and there aren't
any obviously gratuitous code style violations" is the sort of review
I'm looking for.  I'm not sure a great way to get this, but asking on
llvmdev is probably a good start.
>>>> 
>>> 
>>> Hi LLVM Developers,
>>> 
>>> I would like to merge the R600 backend from branches/R600 into TOT
>>> prior to the LLVM 3.2 release.  As you can see in the above
paragraph,
>>> Chris has asked me to find someone to review the code.  If there is
>>> anyone that would be willing to take the time and look at the code,
>>> I would really appreciate it.  For your convenience, I have split
the
>>> code into two patches:
>>> 
>>> http://people.freedesktop.org/~tstellar/llvm/R600.patch
>>> http://people.freedesktop.org/~tstellar/llvm/SI.patch
>>> 
>>> R600.patch adds the code required to support the R600 generation of
GPUs,
>>> and SI.patch adds the code required to support the Southern Islands
>>> generation of GPUs.
>>> 
>>> In addition, this code can be found in the branches/R600 branch of
the
>>> llvm SVN repository, as well as in my git repo:
>>> 
>>> git fetch git://people.freedesktop.org/~tstellar/llvm master
>>> gitweb: http://cgit.freedesktop.org/~tstellar/llvm/
>>> 
>>> In the attached patches, the R600 backend is not built by default,
>>> so you need to pass the --enable-experimental-targets=AMDGPU to
build
>>> it.  If the backend is merged into TOT, I was planning to keep the
>>> build disabled by default, but I don't mind enabling it if
someone wants
>>> to approve that change.
>>> 
>>> I will also be changing the name of the backend directory from
AMDGPU to
>>> R600 per Chris' suggestion.  These patches still use the AMDGPU
>>> directory, but I'll be updating the R600 branch with this
change
>>> as soon as I am able to pass along updated build instructions to
our users.
>>> 
>>> Looking forward to your comments.
>> 
> 
> Hi Ben,
> 
> Thank you for reviewing the code!
> 
>> I went over the patches today and I must say that it looks a lot better
than when you started getting it in shape for inclusion in LLVM. Great Job!
>> 
>> While I can't comment on technical details of the architecture,
here are my high level style comments:
>> 
>> - The prevailing style of brace placement in LLVM is to never put the
opening brace on it's own line. There are different styles used in your
patches.
>> - In some places the indentation uses 4 or more spaces instead of the
preferred two.
>> - There's commented out code in both c++ source and the .td files.
Please either document why it's commented out or remove the code.
"//FIXME: gcc complains on this." isn't helpful at all.
>> - Macros like
>> #define FirstNonDebugInstr(A) A->begin()
>> are not only confusing, the name also says something that the
definition doesn't provide. Please audit the macros and change them to
UPPERCASE so it's obvious that there's some macro magic at work.
>> 
> 
> I've made these changes and also renamed the directory from AMDGPU
> to R600 as suggested by Chris.  An updated patch can be found here:
> http://people.freedesktop.org/~tstellar/llvm/Nov26/
Sorry for being pedantic, but there are still misplaced braces in your patch.
You can easily find them in the source code with:

$ grep -r '^[ ]*{$'

Otherwise I don't see anything else that needs fixing offhand.
> I've also included in that directory the 6 patches I wrote to implement
> these changes, to make it easier to see what is different.  If you see
> anything I left out, let me know.
> 
>> Overall I think this should go in as soon as possible. It has been
around for a while and you have been actively maintaining it over the whole
timespan since it was first proposed for inclusion. I'm afraid it's too
late for 3.2, but we're now early in the 3.3 cycle and it would be really
nice to have a stable R600 backend in 3.3.
>> 
> 
> Assuming I caught all the coding style mistakes you pointed out, what
> else needs to be done to be able to commit this code?
IMO this can go in now, but you'll need approval from Chris.

- Ben

On Tue, Nov 27, 2012 at 12:37 AM, Tom Stellard <tom at stellard.net>
wrote:
> Assuming I caught all the coding style mistakes you pointed out, what
> else needs to be done to be able to commit this code?
Hello Tom,

There are still a few classes of coding style issues...

+  /// CreateLiveInRegister - Helper function that adds Reg to the LiveIn list
+  /// of the DAG's MachineFunction.  This returns a Register SDNode
representing
+  /// Reg.
+  SDValue CreateLiveInRegister(SelectionDAG &DAG, const
TargetRegisterClass *RC,
+                                                  unsigned Reg, EVT VT) const;

Please don't duplicate function or class name inside the comment.
Please also don't duplicate the comment in .cpp file.  Old code is
written that way, but current coding style advises not to duplicate.

This link may be helpful:
<http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments>.

+#ifndef _AMDIL7XXDEVICEIMPL_H_
+#define _AMDIL7XXDEVICEIMPL_H_

Identifiers starting with underscore and a capital letter are reserved.

+  CFGTraits::insertCondBranchBefore(branchInstrPos,
+                                    CFGTraits::getBranchNzeroOpcode(oldOpcode),
+                                    passRep,
+»······»·······»·······»·······»·······»·······»·······»·······»·······branchDL);

No tabs, please.

+AMDGPUCFGStructurizer::AMDGPUCFGStructurizer(char &pid, TargetMachine
&tm
+                                          )
+: MachineFunctionPass(pid), TM(tm), TII(tm.getInstrInfo()),
+  TRI(static_cast<const AMDGPURegisterInfo *>(tm.getRegisterInfo())
+  ) {
+}

Both right parentheses should be on previous lines.

+};   //end of class AMDGPUCFGPrepare

In general I don't see "end of class" and "end of
function" comments
in LLVM, just namespaces.

+// The AMDGPUCypressDevice is similiar to the AMDGPUEvergreenDevice,
except it has
+// support for double precision operations. This device is used to represent
+// both the Cypress and Hemlock cards, which are commercially known as HD58XX
+// and HD59XX cards.
+class AMDGPUCypressDevice : public AMDGPUEvergreenDevice {

Lots of comments should have three slashes (to become documentation comments).

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at
gmail.com>*/

On Thu, Nov 29, 2012 at 06:28:15PM +0200, Dmitri Gribenko
wrote:
> On Tue, Nov 27, 2012 at 12:37 AM, Tom Stellard <tom at stellard.net>
wrote:
> > Assuming I caught all the coding style mistakes you pointed out, what
> > else needs to be done to be able to commit this code?
> 
> Hello Tom,
> 
> There are still a few classes of coding style issues...
> 
> +  /// CreateLiveInRegister - Helper function that adds Reg to the LiveIn
list
> +  /// of the DAG's MachineFunction.  This returns a Register SDNode
> representing
> +  /// Reg.
> +  SDValue CreateLiveInRegister(SelectionDAG &DAG, const
> TargetRegisterClass *RC,
> +                                                  unsigned Reg, EVT VT)
const;
> 
> Please don't duplicate function or class name inside the comment.
> Please also don't duplicate the comment in .cpp file.  Old code is
> written that way, but current coding style advises not to duplicate.
> 
> This link may be helpful:
>
<http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments>.
> 
> +#ifndef _AMDIL7XXDEVICEIMPL_H_
> +#define _AMDIL7XXDEVICEIMPL_H_
> 
> Identifiers starting with underscore and a capital letter are reserved.
> 
> +  CFGTraits::insertCondBranchBefore(branchInstrPos,
> +                                   
CFGTraits::getBranchNzeroOpcode(oldOpcode),
> +                                    passRep,
>
+»······»·······»·······»·······»·······»·······»·······»·······»·······branchDL);
> 
> No tabs, please.
> 
> +AMDGPUCFGStructurizer::AMDGPUCFGStructurizer(char &pid, TargetMachine
&tm
> +                                          )
> +: MachineFunctionPass(pid), TM(tm), TII(tm.getInstrInfo()),
> +  TRI(static_cast<const AMDGPURegisterInfo *>(tm.getRegisterInfo())
> +  ) {
> +}
> 
> Both right parentheses should be on previous lines.
> 
> +};   //end of class AMDGPUCFGPrepare
> 
> In general I don't see "end of class" and "end of
function" comments
> in LLVM, just namespaces.
> 
> +// The AMDGPUCypressDevice is similiar to the AMDGPUEvergreenDevice,
> except it has
> +// support for double precision operations. This device is used to
represent
> +// both the Cypress and Hemlock cards, which are commercially known as
HD58XX
> +// and HD59XX cards.
> +class AMDGPUCypressDevice : public AMDGPUEvergreenDevice {
> 
> Lots of comments should have three slashes (to become documentation
comments).
>
Hi Dimitri,

Thanks for taking a look at the code.

I went ahead and made all of these changes, and I reviewed the doxygen section
of the Coding Standards document and updated the comments as well.
I also fixed the issue mentioned by Ben with braces on their own line. 

I've posted an updated patch here:
http://people.freedesktop.org/~tstellar/llvm/Nov29/

Also in that directory are two patches which show the changes since the
previous version of R600.patch.

Hopefully this patch looks a little better.

Thanks,
Tom

Hi Chris,

Ben and Dmitri have completed coding style reviews for the R600 backend,
and I have made all the requested changes.  What else needs to be done
before we can commit this backend to the main tree?

Thanks,
Tom

On Thu, Nov 29, 2012 at 05:09:57PM +0100, Benjamin Kramer
wrote:
> 
> 
> $ grep -r '^[ ]*{$'
> 
> Otherwise I don't see anything else that needs fixing offhand.
> 
> > I've also included in that directory the 6 patches I wrote to
implement
> > these changes, to make it easier to see what is different.  If you see
> > anything I left out, let me know.
> > 
> >> Overall I think this should go in as soon as possible. It has been
around for a while and you have been actively maintaining it over the whole
timespan since it was first proposed for inclusion. I'm afraid it's too
late for 3.2, but we're now early in the 3.3 cycle and it would be really
nice to have a stable R600 backend in 3.3.
> >> 
> > 
> > Assuming I caught all the coding style mistakes you pointed out, what
> > else needs to be done to be able to commit this code?
> 
> IMO this can go in now, but you'll need approval from Chris.
> 
> - Ben