final graal HSAIL support patch

Christian Thalinger christian.thalinger at oracle.com
Mon Jul 8 14:43:14 PDT 2013 

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