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

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.

Thanks,
Tom Stellard
> > 
> >> Also, it would make more sense to name the directory
lib/Target/R600 than AMDGPU (R600 isn't AMD's only GPU).  :)
> >> 
> > 
> > The naming schemes used for GPUs is really confusing, so I'll try
to
> > explain:
> > 
> > The code in lib/Target/AMDGPU supports two different GPU
architectures.
> > The first is the R600 architecture (in the Open Source drivers we name
> > the architecture after the first GPU family that uses it).  The
> > R600 architecture covers 4 families of GPU: R600, R700, Evergreen,
> > Cayman.
> > 
> > The second is the Graphics Core Next architecture, which we call
> > Southern Islands, this covers the Southern Islands family and possibly
> > future GPU families too.
> > 
> > While these two architectures are completely different when it comes
to
> > registers and ISA, there are some concepts that they share, so it is
> > useful to be able to share code between them.  This is why they are
both
> > in the AMDGPU directory.
> > 
> > If you want I can split these into two separate directories R600, and
> > SI, but then I would need to come up with a some other way to share
code
> > between them.
> 
> Yeah, that is really confusing.  :)  If they really are similar enough to
share code, then I'd still rather that you come up with a name other than
"AMDGPU" for this.  AMD will presumably continue to come out with GPUs
5 or 10 years from now and at SOME point there will be a major divergence. 
I'd rather you give it an inaccurate name like "R600" than an
inaccurate name like "AMDGPU".
> 
> -Chris
>

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.
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.

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.

- Ben

I think it makes perfect sense to get this in now.  That would give Tom and
others an entire release cycle to work out any integration bugs.


On Sat, Nov 17, 2012 at 4:56 PM, Benjamin Kramer <benny.kra at
gmail.com>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.
>
> 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.
>
> 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.
>
> - Ben
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>


-- 

Thanks,

Justin Holewinski
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121118/00cddd7b/attachment.html>

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