[Sw.runtimes] final graal HSAIL support patch

Deneau, Tom tom.deneau at amd.com
Thu Jul 11 13:11:48 PDT 2013 

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.

-- Tom

-----Original Message-----
From: Doug Simon [mailto:doug.simon at oracle.com] 
Sent: Thursday, July 11, 2013 3:01 PM
To: Deneau, Tom
Cc: sw.runtimes; graal-dev at openjdk.java.net
Subject: Re: [Sw.runtimes] final graal HSAIL support patch

On Jul 11, 2013, at 9:35 PM, "Deneau, Tom" <tom.deneau at 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 at mpdtxmail.amd.com [mailto:sw.runtimes-bounces at mpdtxmail.amd.com] On Behalf Of Doug Simon
> Sent: Tuesday, July 09, 2013 4:23 AM
> To: sw.runtimes
> Cc: graal-dev at 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 at 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 at oracle.com] 
>> Sent: Monday, July 08, 2013 4:43 PM
>> To: Venkatachalam, Vasanth
>> Cc: sw.runtimes; graal-dev at openjdk.java.net
>> Subject: Re: final graal HSAIL support patch
>> 
>> 
>> On Jul 8, 2013, at 2:41 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>> 
>>> 
>>> On Jun 28, 2013, at 1:28 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>> 
>>>> 
>>>> On Jun 26, 2013, at 3:00 PM, "Venkatachalam, Vasanth" <Vasanth.Venkatachalam at 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 at oracle.com] 
>>>>> Sent: Monday, June 24, 2013 4:39 PM
>>>>> To: Venkatachalam, Vasanth
>>>>> Cc: sw.runtimes; donald.smith at oracle.com Smith; graal-dev at 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 at 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 at oracle.com] 
>>>>> Sent: Monday, June 24, 2013 3:14 PM
>>>>> To: Venkatachalam, Vasanth
>>>>> Cc: sw.runtimes; donald.smith at oracle.com Smith; graal-dev at 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 at oracle.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> On Jun 24, 2013, at 11:13 AM, Christian Thalinger <christian.thalinger at 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 at 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>
>>>> 
>>> 
>> 
>> 
>> 
> 
> 
> 
>

More information about the graal-dev mailing list