[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-graal-dev
Subject: Re: [Sw.runtimes] final graal HSAIL support patch
From: Christian Thalinger <christian.thalinger () oracle ! com>
Date: 2013-07-12 16:07:04
Message-ID: 8FDE25C4-F1FD-4B9D-8F39-FF7357B5385B () oracle ! com
[Download RAW message or body]
On Jul 11, 2013, at 1:11 PM, "Deneau, Tom" <tom.deneau@amd.com> wrote:
> I have a cr.openjdk.java.net/~tdeneau account. Should I just place the webrev \
> there or do you want to go thru the procedures you used before of me emailing the \
> patch to someone who will put it in some other location for review.
If you have an account just upload your webrev and send the link to this list.
-- Chris
>
> -- Tom
>
> -----Original Message-----
> From: Doug Simon [mailto:doug.simon@oracle.com]
> Sent: Thursday, July 11, 2013 3:01 PM
> To: Deneau, Tom
> Cc: sw.runtimes; graal-dev@openjdk.java.net
> Subject: Re: [Sw.runtimes] final graal HSAIL support patch
>
> On Jul 11, 2013, at 9:35 PM, "Deneau, Tom" <tom.deneau@amd.com> wrote:
>
> > Doug and others ---
> >
> > We have a fairly small amount of bugfixes or new functionality which we added to \
> > the HSAIL backend while the review was going on. And we would like that to get \
> > into the trunk. Should we just supply a webrev with some explanatory text as to \
> > what the changes are accomplishing?
>
> Unfortunately, we don't have many other options in terms of reviewing external \
> submissions (I greatly anticipate the availability of Crucible for OpenJDK \
> projects!). So yes, please submit a webrev.
> > On a separate topic, as you are probably aware from the review, we have some \
> > junit tests that require jdk8 (they use lambdas) and were thus not submitted as \
> > part of the webrev. We actually have considerably more tests that use lambdas \
> > than the ones that don't only because the lambda tests were a little easier to \
> > write.
>
> Unfortunately, we cannot move to Java 8 language features until the tools we use \
> (namely Eclipse) supports these features. If there was a tool for translating Java \
> 8 source code into Java 7 source code, you could at least keep writing the tests \
> with lambdas and check in the generated sources. However, I'm not aware of such a \
> tool. Keep your eyes on http://wiki.eclipse.org/JDT_Core/Java8 because we'll \
> migrate as soon as Eclipse support for Java 8 is reasonably stable.
> > We can of course run these jdk8-based tests privately here but it would be nice \
> > if they could be used to catch problems sooner.
> >
> > What is your estimate as to when graal will support projects that require jdk8?
>
> That depends entirely on the Eclipse effort mentioned above.
>
> -Doug
>
> > -----Original Message-----
> > From: sw.runtimes-bounces@mpdtxmail.amd.com \
> > [mailto:sw.runtimes-bounces@mpdtxmail.amd.com] On Behalf Of Doug \
> > Simon
> > Sent: Tuesday, July 09, 2013 4:23 AM
> > To: sw.runtimes
> > Cc: graal-dev@openjdk.java.net
> > Subject: Re: [Sw.runtimes] final graal HSAIL support patch
> >
> > The integrated patch is now in the OpenJDK repo: \
> > http://hg.openjdk.java.net/graal/graal/rev/4ef92b67aeae
> > The HSAIL simulator page[1] should now be updated as a result. In particular, the \
> > command line for running the test is now:
> > mx --vm server unittest @-G:-RemoveNeverExecutedCode \
> > @-Dsun.boot.library.path=$LD_LIBRARY_PATH @-Xms1g @-Xmx1g @XX:-UseCompressedOops \
> > hsail.test.IntSquaredTest
> > -Doug
> >
> > [1] https://wiki.openjdk.java.net/display/Sumatra/The+HSAIL+Simulator
> >
> > On Jul 9, 2013, at 12:35 AM, "Venkatachalam, Vasanth" \
> > <Vasanth.Venkatachalam@amd.com> wrote:
> > > That HotspotGraalRuntime instance does not have to be public. We had made it \
> > > public a while ago during some internal testing of the AMD64 backend, but I \
> > > thought we had reverted it back in the patch I submitted. You can go ahead and \
> > > make it private.
> > > Vasanth
> > >
> > > -----Original Message-----
> > > From: Christian Thalinger [mailto:christian.thalinger@oracle.com]
> > > Sent: Monday, July 08, 2013 4:43 PM
> > > To: Venkatachalam, Vasanth
> > > Cc: sw.runtimes; graal-dev@openjdk.java.net
> > > Subject: Re: final graal HSAIL support patch
> > >
> > >
> > > On Jul 8, 2013, at 2:41 PM, Christian Thalinger \
> > > <christian.thalinger@oracle.com> wrote:
> > > >
> > > > On Jun 28, 2013, at 1:28 PM, Christian Thalinger \
> > > > <christian.thalinger@oracle.com> wrote:
> > > > >
> > > > > On Jun 26, 2013, at 3:00 PM, "Venkatachalam, Vasanth" \
> > > > > <Vasanth.Venkatachalam@amd.com> wrote:
> > > > > > Hi Christian,
> > > > > >
> > > > > > Attached is our revised patch which makes the changes requested below and \
> > > > > > removes the okra packages.
> > > > > > For the Javadoc, we inserted brief Javadoc comments for every new package \
> > > > > > introduced. We assumed this would be sufficient since you can generate \
> > > > > > the Javadoc html files from the sources. Let us know if anything else is \
> > > > > > needed here.
> > > > > > Please send us a link to the revised webrev once you've posted it.
> > > > >
> > > > > Sorry for the delay:
> > > > >
> > > > > http://cr.openjdk.java.net/~twisti/GRAAL-342/webrev/
> > > >
> > > > Any comments on this patch? If not, I'm going to push this today.
> > >
> > > Oh, wait! Actually I have a question: why do you need this to be public?
> > >
> > > --- old/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/HotSpotGraalRuntime.java 2013-06-28 \
> > > 13:26:17.000000000 -0700
> > > +++ new/graal/com.oracle.graal.hotspot/src/com/oracle/graal/hotspot/HotSpotGraalRuntime.java 2013-06-28 \
> > > 13:26:17.000000000 -0700 @@ -49,7 +49,7 @@
> > > */
> > > public abstract class HotSpotGraalRuntime implements GraalRuntime {
> > >
> > > - private static HotSpotGraalRuntime instance;
> > > + public static HotSpotGraalRuntime instance;
> > >
> > > /**
> > > * Gets the singleton {@link HotSpotGraalRuntime} object.
> > >
> > > -- Chris
> > >
> > > >
> > > > -- Chris
> > > >
> > > > >
> > > > > -- Chris
> > > > >
> > > > > >
> > > > > > Vasanth
> > > > > >
> > > > > > From: Thomas Wuerthinger [mailto:thomas.wuerthinger@oracle.com]
> > > > > > Sent: Monday, June 24, 2013 4:39 PM
> > > > > > To: Venkatachalam, Vasanth
> > > > > > Cc: sw.runtimes; donald.smith@oracle.com Smith; \
> > > > > > graal-dev@openjdk.java.net; Christian Thalinger
> > > > > > Subject: Re: final graal HSAIL support patch
> > > > > >
> > > > > > Yes, I agree. We are working on expanding our automatic checks. \
> > > > > > Additionally, we are working on a style guide document. Given that you \
> > > > > > are the first major outside contributor, this involves a certain learning \
> > > > > > experience for us. - thomas
> > > > > > On Jun 24, 2013, at 11:29 PM, "Venkatachalam, Vasanth" \
> > > > > > <Vasanth.Venkatachalam@amd.com> wrote:
> > > > > >
> > > > > > Hi Thomas,
> > > > > >
> > > > > > We're addressing your comments. It would be nice if checkstyle and/or the \
> > > > > > Graal formatter in Eclipse would have picked up on these errors, so that \
> > > > > > we could have caught them the first time around.
> > > > > > Vasanth
> > > > > >
> > > > > > From: Thomas Wuerthinger [mailto:thomas.wuerthinger@oracle.com]
> > > > > > Sent: Monday, June 24, 2013 3:14 PM
> > > > > > To: Venkatachalam, Vasanth
> > > > > > Cc: sw.runtimes; donald.smith@oracle.com Smith; \
> > > > > > graal-dev@openjdk.java.net; Christian Thalinger
> > > > > > Subject: Re: final graal HSAIL support patch
> > > > > >
> > > > > > Vasanth,
> > > > > >
> > > > > > Thanks for the patch. I have a first round of coding style comments. We \
> > > > > > would like the code to adhere to the style guide used for all other Graal \
> > > > > > modules (although of course we might have already quite some violations \
> > > > > > in the code base...). For explaining some of those style guide lines, I'm \
> > > > > > using the HSAILAssembler.java file as an example [1]:
> > > > > > - Line 36: Comment should end with ".".
> > > > > > - Line 37: Instance field should be declared private.
> > > > > > - Line 37: "= 0" should be removed given that 0 is the default value for \
> > > > > > variables of type "int".
> > > > > > - Line 37: The name of the variable must be written in camel case \
> > > > > > "maxdatatypesize" => "maxDataTypeSize".
> > > > > > - Line 38: Comment should start with an upper case letter.
> > > > > > - Line 56: Remove "// TODO Auto-generated method stub". If the method is \
> > > > > > not used, replace with throwing an error/exception \
> > > > > > (GraalInternalError.shouldNotReachHere).
> > > > > > - Line 60: Method name "at" seems too short/ambiguous.
> > > > > > - Line 72-76: Multi-line "//" comment should be replaced with javadoc \
> > > > > > "/** */" comment.
> > > > > > - Line 88: Remove unnecessary blank line.
> > > > > > - Line 91: Use camel case for method names "emit_mov" => "emitMov" (we \
> > > > > > have violated this rule for the PTX assembler, but are going to fix that; \
> > > > > > at least we want to use "_" only in rare instances).
> > > > > > - Line 97: Remove unnecessary blank line (please also fix for subsequent \
> > > > > > lines of the file).
> > > > > > - Line 135: No commented-out code - please remove line.
> > > > > > - Line 190: Use camel case for method names "emit_convert" => \
> > > > > > "emitConvert" (please also fix for subsequent method \
> > > > > > names of the file).
> > > > > > - Line 196: Either remove comment or put javadoc comment including \
> > > > > > additional information.
> > > > > > - Line 236: Comment on method should be javadoc comment.
> > > > > > - Line 247-248: Comment seems on the wrong place. If this is a comment on \
> > > > > > the parameter "controlRegString", then please put the comment in the \
> > > > > > javadoc.
> > > > > > Looking over the other files, there are similar style guide line \
> > > > > > violations, so please make a short sweep over the patch with those items \
> > > > > > in mind.
> > > > > > Additionally, we would like to see a short javadoc description for each \
> > > > > > newly added class. This seems important given that we will need to \
> > > > > > maintain this code moving forward.
> > > > > > Thanks, thomas
> > > > > >
> > > > > > [1] http://cr.openjdk.java.net/~twisti/GRAAL-342/webrev/graal/com.oracle.graal.asm.hsail/src/com/oracle/graal/asm/hsail/HSAILAssembler.java.html
> > > > > >
> > > > > > On Jun 24, 2013, at 8:18 PM, Christian Thalinger \
> > > > > > <christian.thalinger@oracle.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Jun 24, 2013, at 11:13 AM, Christian Thalinger \
> > > > > > <christian.thalinger@oracle.com> wrote:
> > > > > >
> > > > > >
> > > > > > I've created a Graal tracking bug so it's easier for me to handle it (the \
> > > > > > bug is not visible outside of Oracle):
> > > > > > GRAAL-342: add HSAIL backend
> > > > > >
> > > > > > Here is the new webrev:
> > > > > >
> > > > > > http://cr.openjdk.java.net/~twisti/GRAAL-342/webrev/
> > > > > >
> > > > > > Two quick comments:
> > > > > >
> > > > > > 1) I understand that the way you're handling the assembler right now was \
> > > > > > much less work but I wonder if you ever want to go to support your binary \
> > > > > > format too. Then a full-fledged assembler as for the other platforms \
> > > > > > might be better.
> > > > > > 2) The Labels. I remember talking to Gilles about this when I did the \
> > > > > > PTX work and I think there is a better way of doing this (although I \
> > > > > > can't recall).
> > > > > > -- Chris
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > -- Chris
> > > > > >
> > > > > > On Jun 19, 2013, at 3:27 PM, "Venkatachalam, Vasanth" \
> > > > > > <Vasanth.Venkatachalam@amd.com> wrote:
> > > > > >
> > > > > >
> > > > > > Hi Christian,
> > > > > >
> > > > > > Here's the final version of our graal HSAIL support patch. This addresses \
> > > > > > all of Doug Simon's comments.
> > > > > > In particular,
> > > > > >
> > > > > > 1) Checkstyle errors have been fixed and all licenses in the Okra \
> > > > > > packages are attributed to Oracle. 2) What was previously the \
> > > > > > com.amd.sumatra package has been removed and its source files (which \
> > > > > > don't have anything Sumatra specific) have been moved into the \
> > > > > > com.oracle.graal.compiler.hsail package. 3) Hotspot c++ source \
> > > > > > changes have been reverted and are no longer part of the webrev.
> > > > > > Vasanth
> > > > > > <graal.patch>
> > > > > >
> > > > > >
> > > > > >
> > > > > > <graal.zip>
> > > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic