RFR(M): Graal PTX enhancements
Morris Meyer
morris.meyer at oracle.com
Tue Apr 9 16:35:42 PDT 2013
Thomas - thanks for the review. Here is the update.
--mm
WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.06
On 4/9/13 5:24 PM, Thomas Wuerthinger wrote:
> Answers inline. Thanks, thomas
>
> On Apr 9, 2013, at 10:04 PM, Morris Meyer <morris.meyer at oracle.com> wrote:
>> On 4/9/13 2:14 PM, Thomas Wuerthinger wrote:
>>> Morris,
>>>
>>> Looks good. Here are some comments:
>>>
>>> - In PTXLIRGenerator.emitConvert: I think you do not need the switch statement as in all cases, you are just passing on the operation to the created LIR instruction. Are L2I and I2C really so different that they are the only ones needing an Unary1Op?
>> I pulled this from AMD64LIRGenerator.emitConvert. Perhaps we could refactor both in a later change set?
> OK. I still think that you should use Unary2Op for all operations here. The difference between Unary1Op and Unary2Op is that the former requires source and destination register to be the same where as the latter allows them to be different. I believe PTX supports for all convert operations to use a destination operand that is different from the source operand.
>
>>> - In the ControlTest class: There are several lines commented out. Please either uncomment them or remove them from the patch. We have the policy that there should not be any commented out code in the repository.
>> Fixed.
>>> - In the PTXTestBase class: Why are you using an @Ignore annotation on the class? I think we should run the PTX test as soon as possible together with our regular unit tests, only ignoring the ones we know to fail.
>> The @Ignore is needed as without it junit fails as there are no test cases in PTXTestBase.
> You should rather declare it abstract. Then JUnit will not instantiate the test class.
>
>>> - In the PTXPhase class: Maybe you can add a comment why it is safe to assume all object parameters as non-null for PTX kernels - and this is the purpose of this phase.
>> # HG changeset patch
>> # User Thomas Wuerthinger <thomas.wuerthinger at oracle.com>
>> # Date 1362480569 -3600
>> # Node ID 9ac11c77d12858f742e00455f1f680be538d3c8b
>> # Parent 47a7e8d8053684bfb060b1a0ff4ac59fa69eace9
>> Mark PTX parameters as non-null
>>
>> Could you suggest the text - as you put this phase into the BasicPTXTest originally?.
> OK. My assumption was that at the point when somebody invokes a PTX kernel, the null checks would be done on the caller side (i.e., before copying the data onto the GPU). Do you think that assumption is valid and makes sense?
>
>> WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.05
>>> Thanks, thomas
>>>
>>>
>>> On Apr 9, 2013, at 8:02 PM, Morris Meyer <morris.meyer at oracle.com> wrote:
>>>
>>>> Gilles,
>>>>
>>>> Fixed in this updated version. Thanks much.
>>>>
>>>> --mm
>>>>
>>>> WEBREV - http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.04
>>>>
>>>> On 4/9/13 11:24 AM, Gilles Duboscq wrote:
>>>>> It's not so important but there are still some System.err.println (in PTXLIRGenerator.java, PTXCompare.java and PTXControlFlow.java)
>>>>>
>>>>>
>>>>> On Tue, Apr 9, 2013 at 5:03 PM, Morris Meyer <morris.meyer at oracle.com <mailto:morris.meyer at oracle.com>> wrote:
>>>>>
>>>>> Thanks Gilles, Doug and Christian for the review. Here is my
>>>>> updated webrev addressing those concerns.
>>>>>
>>>>> --mm
>>>>>
>>>>> WEBREV -
>>>>> http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.03
>>>>> <http://cr.openjdk.java.net/%7Emorris/Graal-PTX-Enhancements.03>
>>>>>
>>>>>
>>>>> On 4/9/13 7:59 AM, Gilles Duboscq wrote:
>>>>>> There are some System.(out|err).println in various files could
>>>>>> you remove them? When there are things like
>>>>>> throw GraalInternalError.shouldNotReachHere(), if you need some
>>>>>> output you can also
>>>>>> use GraalInternalError.shouldNotReachHere(String message) instead
>>>>>> of the println
>>>>>>
>>>>>> Otherwise it looks good
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 9, 2013 at 1:59 PM, Gilles Duboscq
>>>>>> <gilwooden at gmail.com <mailto:gilwooden at gmail.com>> wrote:
>>>>>>
>>>>>> There are some System.(out|err).println in various files
>>>>>> could you remove them? When there are things like
>>>>>> throw GraalInternalError.shouldNotReachHere(), if you need
>>>>>> some output you can also
>>>>>> use GraalInternalError.shouldNotReachHere(String message)
>>>>>> instead of the println
>>>>>>
>>>>>> Otherwise it looks good
>>>>>>
>>>>>>
>>>>>> On Sat, Apr 6, 2013 at 1:18 AM, Morris Meyer
>>>>>> <morris.meyer at oracle.com <mailto:morris.meyer at oracle.com>> wrote:
>>>>>>
>>>>>> Folks
>>>>>>
>>>>>> There is an updated webrev with Christian's review
>>>>>> changes here:
>>>>>>
>>>>>> WEBREV -
>>>>>> http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.02
>>>>>> <http://cr.openjdk.java.net/%7Emorris/Graal-PTX-Enhancements.02>
>>>>>>
>>>>>> --morris
>>>>>>
>>>>>>
>>>>>> On 4/5/13 6:09 PM, Morris Meyer wrote:
>>>>>>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> Actually with JavaFX, explicit class declarations
>>>>>> were part of our check-in procedures. I put the
>>>>>> changes into PTXLIRGenerator as IMHO it helps folks
>>>>>> just looking at the source to see what makes up the
>>>>>> class.
>>>>>>
>>>>>> Will change the class to PTXTestBase and look after
>>>>>> the indenting.
>>>>>>
>>>>>> --mm
>>>>>>
>>>>>>
>>>>>> On Apr 5, 2013, at 5:42 PM, Christian Thalinger
>>>>>> <christian.thalinger at oracle.com
>>>>>> <mailto:christian.thalinger at oracle.com>> wrote:
>>>>>>
>>>>>> On Apr 5, 2013, at 10:46 AM, Morris Meyer
>>>>>> <morris.meyer at oracle.com
>>>>>> <mailto:morris.meyer at oracle.com>> wrote:
>>>>>>
>>>>>> Folks,
>>>>>>
>>>>>> Could I get a review of my recent PTX
>>>>>> enhancements to Graal? I've changed the
>>>>>> tests to have a base class, extended the
>>>>>> assembler, and have taken the test coverage
>>>>>> of PTX from 2 to 23 tests.
>>>>>>
>>>>>> Thanks in advance,
>>>>>>
>>>>>> --morris meyer
>>>>>>
>>>>>> WEBREV -
>>>>>> http://cr.openjdk.java.net/~morris/Graal-PTX-Enhancements.01
>>>>>> <http://cr.openjdk.java.net/%7Emorris/Graal-PTX-Enhancements.01>
>>>>>>
>>>>>> This is great!
>>>>>>
>>>>>> Looking at:
>>>>>>
>>>>>> graal/com.oracle.graal.compiler.ptx/src/com/oracle/graal/compiler/ptx/PTXLIRGenerator.java
>>>>>>
>>>>>> it seems we need to turn off "Organize imports"
>>>>>> in our favorite editor. Also indenting seems to
>>>>>> be an issue.
>>>>>>
>>>>>> I know the answer is "use the mx script to create
>>>>>> the IDE configuration" but no everybody is using
>>>>>> Eclipse or Netbeans. We need to find a way to
>>>>>> keep this consistent without the help of an IDE
>>>>>> (maybe Mercurial push hooks?).
>>>>>>
>>>>>> graal/com.oracle.graal.compiler.ptx.test/src/com/oracle/graal/compiler/ptx/test/PTXCompilerBase.java:
>>>>>>
>>>>>> Can we name the test base class PTXTestBase?
>>>>>>
>>>>>> Otherwise this looks good to me.
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>
>>>>>>
>>>>>>
More information about the graal-dev
mailing list