HSAIL Backend: Register getting overwritten as part of Move To Phi
Doug Simon
doug.simon at oracle.com
Fri Jul 19 14:18:00 PDT 2013
Yep.
Sent from my iPhone
On Jul 19, 2013, at 10:09 PM, "Deneau, Tom" <tom.deneau at amd.com> wrote:
> Doug --
>
> So I assume this is the following changeset which has already been pushed?
>
> -- Tom
>
> Changeset: a61fa3e171e7
> Author: Doug Simon <doug.simon at oracle.com>
> Date: 2013-07-19 12:45 +0200
> URL: http://hg.openjdk.java.net/graal/graal/rev/a61fa3e171e7
>
> fixed bug in EdgeMoveOptimizer triggered by a backend (such as HSAIL) that has conditional branches with explicit input operands (as opposed to an implicit condition flags register)
>
> ! graal/com.oracle.graal.lir/src/com/oracle/graal/lir/EdgeMoveOptimizer.java
>
>
>
> -----Original Message-----
> From: Doug Simon [mailto:doug.simon at oracle.com]
> Sent: Friday, July 19, 2013 5:55 AM
> To: Deneau, Tom
> Cc: graal-dev at openjdk.java.net
> Subject: Re: HSAIL Backend: Register getting overwritten as part of Move To Phi
>
> Tom,
>
> I found the bug and am pushing through a fix now.
>
> The issue was that, until now, we've only had backends whose conditional branch instructions operate on a conditional flag set by a preceding (compare) instruction. As such, we never considered (input) operands when moving instructions from successors to in front of a conditional branch. This assumption does not apply to HSAIL (or to PTX?). I've made a change that disables the optimization for conditional branches that have operands.
>
> -Doug
>
> On Jul 17, 2013, at 7:27 PM, "Deneau, Tom" <tom.deneau at amd.com> wrote:
>
>> The problem described below was first noticed last week but then I thought
>> it went away with the large number of checkins on July 15. But now I see
>> that it is still there.
>>
>> I am not sure how to debug this problem so looking for any suggestions.
>> To help you reproduce the problem on your side, I have a small webrev that
>> adds the failing test case. The test case itself uses String.contains and so
>> needed the capability to properly handle accesses to char arrays and char fields,
>> so in addition to the failing test case there is a very small change to the HSAIL
>> backend to support the narrower object types like char.
>>
>> The webrev can be found at
>> http://cr.openjdk.java.net/~tdeneau/graal-webrevs/graal-webrev4.0/
>>
>> One other change in this webrev, I made a change to the okra library loading code
>> originally suggested by Doug. Doug was suggesting a new environment variable pointing
>> to the native library so we wouldn't need the -Dsun.boot.library.path on the java command
>> line. Instead I noticed that we were already requiring the directory to be in the PATH
>> environment variable, so the loading code just looks down the PATH.
>>
>> In the webrev, this change is simply reflected in a change to the version
>> of the okra jar file (since the okra code is not part of graal proper).
>> The change is entirely in the java code, there is no need
>> to rebuild the actual native libraries.
>>
>> Reproducing the failing test case...
>>
>> With the patch applied, you should be able to get the test case failure with the following:
>>
>> * mx --vm server build product
>> * mx --vm server unittest @-G:InlineEverything @-Dkerneltester.logLevel=INFO hsail.test.StringContainsAcceptTest
>>
>> The logLevel=INFO dumps the generated HSAIL to stdout. When run on my system, the problem
>> code starts at @L10. We are reading the first char from each of the strings and
>> getting ready to compare them.
>>
>> @L10:
>> ld_global_u16 $s9, [$d0 + 24];
>> ld_global_u16 $s10, [$d3 + 24];
>> st_spill_s32 $s10, [%spillseg][24];
>> mov_b32 $s9, 0; <<< problem
>> cmp_ne_b1_s32 $c0, $s9, $s10;
>>
>>
>> -- Tom Deneau
>>
>>
>>
>> -----Original Message-----
>> From: Deneau, Tom
>> Sent: Monday, July 15, 2013 4:05 PM
>> To: graal-dev at openjdk.java.net
>> Subject: RE: graal/graal: 107 new changesets
>>
>> Doug or others --
>>
>> I was about to ask for help on a codegen problem we were seeing but after updating to the latest default,
>> I see that it is gone (so I'm assuming it was not in our backend :) ). Of course it is possible that the bug
>> is just being hidden by some unrelated change. Anyway, here is a brief description,
>> maybe you can tell which of the many changesets below would have fixed this, if any.
>>
>> We were using String.contains as our test case, which was being nicely inlined, and we generated the following
>> HSAIL code which led to the problem
>>
>> ld_global_u16 $s8, [$d3 + 24]; // read a u16 char from the test String
>> ld_global_u16 $s9, [$d1 + 24]; // read a u16 char from the pattern
>> st_spill_s32 $s9, [%spillseg][24]; // register spill
>> mov_b32 $s8, 0; // <<<<--- This code was causing the problem
>> cmp_ne_b1_s32 $c0, $s8, $s9; // compare the two chars but s8 has been clobbered
>>
>>
>> When I did a -G:TraceLIRGeneratorLevel=2, I could see that the problematic instruction
>> "mov_b32 $s8, 0"
>> was generated as part of something called
>> MOVE TO PHI from 276|EndNode to 277|LoopBegin
>>
>> as part of PhiResolver.dispose, the part that is commented with
>> // generate move for move from non variable to arbitrary destination
>>
>> Does the above sound like something that was purposely fixed?
>>
>> -- Tom Deneau
>
>
>
More information about the graal-dev
mailing list