update on graal HSAIL backend webrev

Tue Jun 18 12:33:06 PDT 2013

Hi Vasanth,

Here's a proposal that should allow us to integrate your HSAIL backend changes and tests and avoids all licensing issues.

As I understand it, the HSAIL tests depend on the OKRA framework. If you can make this framework available as a jar located at a publicly available URL, then you can simply specify it as a library dependency for the projects that need it (just like we specify the JUNIT library) in mx/projects. This way, the library will be pulled down and cached locally on demand for running the tests.

In terms of the remaining code, we would like to receive and maintain the Graal HSAIL backend (plus tests) in the Graal repository which only contains JDK7 code and is only tested in a JDK7 environment.

For the JDK8 changes, we propose that they remain in a Sumatra downstream repository. This would include the HotSpot code to do the optional interception on the Streams API.

If this proposal is not acceptable, we're open to further discussion. If it is acceptable, could you please create a JDK7 compliant patch that has the Graal HSAIL backend, the tests for those changes and a dependency on OKRA as an external jar library.

-Doug

On Jun 18, 2013, at 6:27 PM, "Venkatachalam, Vasanth" <Vasanth.Venkatachalam at amd.com> wrote:

> Hi,
> 
> I wanted to give an update on our Graal webrev.
> 
> We have fixed all of the Java warnings and most of the checkstyle errors. The only checkstyle errors that remain are coming from the license header used in two packages ( HSAIL simulator source) that currently require a University of Illinois license, as opposed to the Oracle license which checkstyle is expecting. We are internally discussing how to handle the licensing issue.
> 
> I ran the three commands the Doug listed:
> 
> mx clean
> mx build --jdt /path/to/ecj.jar --jdt-warning-as-error 
> mx checkstyle
> 
> Aside from the checkstyle warnings mentioned above, there were no issues. However, when using JDK7, mx still tries to build JDK 8 projects, even though their compliance level was specified as Java 8 in mx/projects. I reported this bug on the mailing list.
> 
> I'd like to propose two possible ways we can submit the revised webrev, and get people's feedback on what's preferable.
> 
> 1) The easiest approach would be to give you a single webrev encompassing all changes and provide you with adequate documentation that will allow you to be selective in what  projects to review. 
> 
> 2) We can split this up into multiple webrevs. However, due to dependencies you will need to apply all the patches to be able to build. The four-way split I propose is as follows:
> 
> A) Hotspot changes
> 
> B) Graal changes
> 	B1) Infrastructure changes
> 	B2) Test cases
> 
> Let me know what's preferable. If I don't hear from anyone I plan to produce 1) within the next couple of days.
> 
> Vasanth
> 
> 
> 
> /home/tester/graalcloneinternal/graal/graal/com.amd.okra/src/com/amd/okra/OkraKernel.java:2: Line does not match expected header line of ' \* Copyright \(c\) (20[0-9][0-9], )?20[0-9][0-9], Oracle and/or its affiliates. All rights reserved.'.
> /home/tester/graalcloneinternal/graal/graal/com.amd.okra/src/com/amd/okra/OkraContext.java:2: Line does not match expected header line of ' \* Copyright \(c\) (20[0-9][0-9], )?20[0-9][0-9], Oracle and/or its affiliates. All rights reserved.'.
> /home/tester/graalcloneinternal/graal/graal/com.amd.okra/src/com/amd/okra/OkraOpenCLtoHSAILCompiler.java:2: Line does not match expected header line of ' \* Copyright \(c\) (20[0-9][0-9], )?20[0-9][0-9], Oracle and/or its affiliates. All rights reserved.'.
> /home/tester/graalcloneinternal/graal/graal/com.amd.okra/src/com/amd/okra/OkraUtil.java:2: Line does not match expected header line of ' \* Copyright \(c\) (20[0-9][0-9], )?20[0-9][0-9], Oracle and/or its affiliates. All rights reserved.'.
> 
> 
> -----Original Message-----
> From: Doug Simon [mailto:doug.simon at oracle.com] 
> Sent: Saturday, June 15, 2013 3:20 PM
> To: Venkatachalam, Vasanth
> Cc: graal-dev at openjdk.java.net
> Subject: Re: webrev for Graal HSAIL backend
> 
> Hi Vasanth,
> 
> On Jun 14, 2013, at 10:34 PM, "Venkatachalam, Vasanth" <Vasanth.Venkatachalam at amd.com> wrote:
> 
>> Hi Doug,
>> 
>> Thanks for the feedback. We're addressing your comments and had a few questions.
>> 
>> 1)  I'm trying to understand the issue with the formatting. To apply the Eclipse formatter, I first ran mx ideinit, then started eclipse and under projects->properties->Java Code Style->Formatter, I made sure the profile (Unmanaged profile 'Graal') was selected. Is this the correct way to apply the formatter?
> 
> Yes. The Eclipse project configurations created by eclipseinit should also apply the formatter to any Java file when it is saved.
> 
>> Can you give an example of some code that wasn't formatted according to these rules?
> 
> Now that I've looked at the patch in more detail, I can't see any problem in terms of formatting.
> 
>> 2). The only projects that use JDK8 features are com.amd.sumatra, and com.oracle.graal.compiler.hsail.test.lambda, In mx/projects we have specified that these projects have Java compliance of 1.8, so they shouldn't be compiled if the JDK used is 1.7. It seems like there should be a way for Eclipse to pick up on these changes after running mx ideinit. Are you saying this isn't the case?
> 
> One problem is that Eclipse doesn't (yet) support JDK8 *language* features such as lambdas. Projects that only use JDK8 library features should be fine.
> 
> The ideinit command does not consider compliance level when generating Eclipse project configurations - it probably should. And it should probably ignore any projects whose compliance level is >= 8 altogether. Not only does Eclipse not yet support JDK8 anguage features, it cannot yet generate JDK8 class files (the maximum "Compiler compliance level" one can currently set in Eclipse is 1.7).
> 
> I've just modified the checkstyle command to pay attention to compliance levels. With this change, you should now be able to test your changes for Graal compliance on the command line with these commands:
> 
> mx clean
> mx build --jdt /path/to/ecj.jar --jdt-warning-as-error mx checkstyle
> 
> Once all 3 pass without any warnings or errors, the code should be easier to review and integrate. Note that these commands are part of our gate so we need to get them passing anyway. If you are game, you could try and replicate our complete gate testing with:
> 
> mx --jacoco on gate
> 
> BTW, if you download an ecj.jar[1] to <graal>/mx/ecj.jar, it will be automatically detected and used by the 'mx build' command, removing the need for the --jdt argument.
> 
> Let me know if you run into any problems or have any further questions.
> 
> -Doug
> 
> [1] http://www.eclipse.org/downloads/download.php?file=/eclipse/downloads/drops4/R-4.2.2-201302041200/ecj-4.2.2.jar
> 
>> -----Original Message-----
>> From: graal-dev-bounces at openjdk.java.net 
>> [mailto:graal-dev-bounces at openjdk.java.net] On Behalf Of Doug Simon
>> Sent: Thursday, June 13, 2013 9:04 AM
>> To: Deneau, Tom
>> Cc: sumatra-dev at openjdk.java.net; graal-dev at openjdk.java.net
>> Subject: Re: webrev for Graal HSAIL backend
>> 
>> Tom,
>> 
>> It would really help to have this patch broken up into components that can be separately considered for integration. In particular (as Morris stated), having the HSAIL backend (and tests) in Graal would be great so a separate patch for this code would be a good first step.
>> 
>> One immediate observation is that a lot of the code does not yet import into Eclipse very well. This is the primary tool we use for development and code comprehension so getting the code into Eclipse compliant form would be very helpful in terms of being able to efficiently assess a contribution and offer further feedback. This means:
>> 
>> o Only use Java 7 language features (Eclipse does not yet support Java 8 language features and it's not clear when it will).
>> o Remove all Checkstyle warnings (use the eclipse-cs plugin or 'mx checkstyle').
>> o Remove all Java warnings (which show up in the Eclipse Problems view).
>> o Format Java code with the Eclipse formatter (the Eclipse projects generated by 'mx eclipseinit' will ensure the Graal code formatting rules are used).
>> 
>> It would be useful to have a high level description of the code, broken down by package or collection of packages. This can either be plain text or (preferably) package-info.java files.
>> 
>> I notice there are files without licenses and some with University of Illinois licenses (the Okra framework). I'm no expert on licensing, but this may causes integration issues.
>> 
>> The HotSpot changes are probably best to keep in the Sumatra repository for now given their experimental nature.
>> 
>> I look forward to further progress in terms of getting Sumatra code into Graal - thanks for a good start!
>> 
>> -Doug
>> 
>> On Jun 12, 2013, at 5:59 PM, "Deneau, Tom" <tom.deneau at amd.com> wrote:
>> 
>>> Morris --
>>> 
>>> Regarding your comments on the hotspot changes in this webrev 
>>> http://cr.openjdk.java.net/~ecaspole/graal_hsail/
>>> I wanted to let you know that these hotspot changes really don't make 
>>> sense on their own but co-operate with some JDK changes which we will 
>>> be submitting in a separate webrev to the Sumatra-dev repository 
>>> early next week (these JDK changes are much smaller than the graal changes).
>>> 
>>> Here is a brief overview of how the pieces fit together...
>>> 
>>> Basically we wanted the GPU offload programming model to be triggered 
>>> by the programmer using Stream.parallel.forEach(). So the JDK changes 
>>> are really just interceptions of the stream API for parallel forEach.
>>> The intercept code tests whether the stream meets a few criteria to 
>>> be offloadable, and if so tell graal (thru the Sumatra interface) to 
>>> compile the lambda method to HSAIL and dispatch it.  If already 
>>> compiled, it will just be dispatched.  Currently we have a property, 
>>> off by default, which tells the JDK intercept code to offload 
>>> immediately.  If the offload-immediate flag is set, no hotspot 
>>> changes are really needed.
>>> 
>>> The hotspot changes just provide an alternate way of enabling the 
>>> offloading (without using the offload-immediate flag) by using the 
>>> compilation of the underlying lambda method as a trigger for 
>>> offloading.  They are very experimental and we would welcome 
>>> community input on other ways to do this.
>>> 
>>> Hope this helps...
>>> 
>>> -- Tom Deneau
>>> 
>>> 
>>> 
>>> -----Original Message-----
>>> From: graal-dev-bounces at openjdk.java.net
>>> [mailto:graal-dev-bounces at openjdk.java.net] On Behalf Of Morris Meyer
>>> Sent: Tuesday, June 11, 2013 7:17 PM
>>> To: graal-dev at openjdk.java.net
>>> Subject: Re: webrev for Graal HSAIL backend
>>> 
>>> Vasanth,
>>> 
>>> After seeing Apple's WWDC and the 4,000 core dual-GPU system they 
>>> built into the Mac Pro, I'm very happy to see the work your team has 
>>> put together.  Lots of good stuff here and I think we should take most of it.
>>> 
>>> I like that the HSAIL backend is in the com.oracle.graal namespace - 
>>> not so much as an Oracle engineer - but it will make working and 
>>> refactoring these GPU and CPU backends much easier.  Thanks.
>>> 
>>> compilerBroker.cpp, library_call.cpp, runtime.cpp and arguments.cpp seem 
>>> like the might be a little specialized for this point in time.   Very 
>>> interesting changes though.   I would like to get the heterogeneous 
>>> method support into HotSpot / Graal and sit down at the language 
>>> summit and discuss how we take on constructs like the ForEach work.
>>> 
>>> I think com.amd.sumatra.Sumatra is sort of in the right ballpark - it 
>>> echos the change I made to CompilerToGPU in the earlier PTX work.  I 
>>> would like of like to reserve the sumatra package for lambda work 
>>> along the lines you are thinking for a collections / lambda oriented
>>> java.lang.invoke set of code.   We need to get the requirements for this 
>>> sort of externalized kernel creating defined as soon as we can.  
>>> Maybe ...bridge.gpu, ...bridge.gpu.hsail, ...bridge.gpu.ptx packages 
>>> in the graal namespace?
>>> 
>>> I think it might be a good next step to put in the HSAIL back end and 
>>> tests and the emulator working at the gate so we can build and verify
>>> JDK9 / Sumatra / Graal changes in this environment going forward.
>>> 
>>> I will be working as time permits on the heterogeneous methods and 
>>> PTX invocation so we can get both platforms at the gate integrating changes.
>>> 
>>> That's all for now.  I'm looking forward to working my way through 
>>> all these unit tests.  Huge kudos AMD!
>>> 
>>>       --morris meyer
>>> 
>>> On 6/11/13 6:16 PM, Venkatachalam, Vasanth wrote:
>>>> Hi,
>>>> 
>>>> The AMD Sumatra team has submitted a webrev (http://cr.openjdk.java.net/~ecaspole/graal_hsail/) that adds HSAIL code generation support for Graal, allowing Java programs to be compiled and executed on HSAIL-enabled GPU/APU devices. While this work is a prototype, we have included several working unit test cases, including Mandelbrot and NBody.
>>>> 
>>>> Features
>>>> 
>>>> Arithmetic operations for integers, longs, doubles, and floats 
>>>> Loads, stores and move operations Min/max/rem/carry operations for 
>>>> integers and longs Conversion operations - (currently support 
>>>> conversions between integers and floats, integers and doubles, integers and longs, floats and doubles).
>>>> Some math library operations (e.g., square root).
>>>> Support for JDK8 lambda constructs.
>>>> 
>>>> Known Issues
>>>> 
>>>> -The logic to handle register spilling is work-in-progress, so not all test cases that induce spilling are guaranteed to work.
>>>> -X86 register encodings are being passed to the HSAIL backend. The 
>>>> calling convention returned by getCallingConvention() currently returns an x86 calling convention -Function call support has yet to be implemented.
>>>> 
>>>> For a detailed list of unsupported features, refer to the routines 
>>>> that are emitting "NYI" in HSAILLIRGenerator.java
>>>> 
>>>> The test cases (except for BasicHSAILTest) require an HSAIL simulator or hardware to execute, but in lieu of a simulator or hardware they will output the HSAIL code generated, which is useful for debugging. Moreover, BasicHSAILTest provides a template for adding Java code snippets and viewing the HSAIL generated code without executing the code.
>>>> 
>>>> We encourage the community to support this new backend and extend it with additional features.
>>>> 
>>>> Vasanth
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 
>> 
>> 
>> 
> 
> 
> 