[Sw.runtimes] final graal HSAIL support patch

[Doug Simon]

Hi Tom,

Ok, based on the need to link with the native code and the fact that there is nothing particularly Graal specific in this code, I think the okra-<version>.jar solution is the best one moving forward. The <version> suffix on the jar should be sufficient to manage any version dependencies from the other HSAIL code and the Okra library. I'm guessing that this library interface will not change very often anyway. As you state, the issue of staying in sync with the native code will have to be resolved some other way anyway.

One request I have is that the okra-<version>.jar file also contain the Java sources so that debugging through this code should be straightforward.

-Doug

On Jun 24, 2013, at 11:08 PM, "Deneau, Tom" <tom.deneau at amd.com> wrote:

> Doug --
> 
> As you may have noticed, many of the methods in com.amd.okra are native so there is a corresponding native library with com_amd_okra_xxx names which is needed if you want to actually dispatch anything.  This is an open source project of its own and will be used by other projects such as Aparapi.  Since there is nothing graal-specific in the native library, it doesn't seem right to have a graal namespace on it.  So leaving com.amd.okra as the package name would be our first choice.
> 
> A second choice would be to have the com.oracle.graal.okra which you propose be a stub which could be built against but would use reflection to get to the real com.amd.okra which could be shipped as a jar with the okra native libraries.
> 
> A third choice is going back to your earlier proposal of graal building against an okra.jar which would be downloaded from somewhere (would cr.openjdk.java.net do for this?)
> 
> Our earlier concern was that the third choice would more easily get out of sync with the actual native libraries but as I think about it all three choices have this possibility of getting out of sync with the native libraries.
> 
> -- Tom
> 
> 
> -----Original Message-----
> From: sw.runtimes-bounces at mpdtxmail.amd.com [mailto:sw.runtimes-bounces at mpdtxmail.amd.com] On Behalf Of Doug Simon
> Sent: Monday, June 24, 2013 3:30 PM
> To: Thomas Wuerthinger
> Cc: graal-dev at openjdk.java.net; sw.runtimes; donald.smith at oracle.com Smith
> Subject: Re: [Sw.runtimes] final graal HSAIL support patch
> 
> Vasanth,
> 
> Just one more request on top of Thomas' feedback.
> 
> We'd like to keep all the Graal code in a com.oracle.graal package namespace. As such, could you please rename the com.amd.okra package to something like com.oracle.graal.amd.okra or simply com.oracle.graal.okra.
> 
> Thanks.
> 
> -Doug
> 
> On Jun 24, 2013, at 10:13 PM, Thomas Wuerthinger <thomas.wuerthinger at oracle.com> wrote:
> 
>> 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>
>>>> 
>>> 
>> 
> 
> 
> 
>