final graal HSAIL support patch
thomas.wuerthinger at oracle.com
Mon Jun 24 13:13:58 PDT 2013
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 :
- 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.
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:
> 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.
More information about the graal-dev