HSAIL Backend: Register getting overwritten as part of Move To Phi

[Doug Simon]

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
> 
> 
>