regression for compressed oops that require shift and add

[Roland Schatz]

Tom,

You're right, this is a regression in Graal. The "instanceof 
ObjectStamp" tests in CompressionNode and StampTool should actually be 
"instanceof AbstractObjectStamp".
I wonder why we're never hitting this bug in the AMD64 backend, at least 
on some of our benchmarks we have a heap base != 0.

I'm currently pushing a fix.

-- Roland

On 06/03/2014 12:28 AM, Deneau, Tom wrote:
> Doug --
>
> To reproduce the failure you can run this single junit test
>     mx unittest -Xmx31g  IntStreamNullCheckNonZeroBciTest
>
> This test purposely passes in a null object pointer in its input array.
>
> It looks like the problem is in CompressionNode.generate
>          if (getInput().stamp() instanceof ObjectStamp) {
>              nonNull = StampTool.isObjectNonNull(getInput().stamp());
>          } else {
>              // metaspace pointers are never null
>              nonNull = true;
>          }
>
> The stamp is NarrowOopStamp which now extends AbstractObjectStamp, so nonNull is always true,
> and in the hsail LIR code we don't generate the code to test for null.
>
> I don't know the best way to fix this.
> Note that if we replace ObjectStamp above with AbstractObjectStamp, there is then another
> similar test in StampTool.isObjectNonNull.
>
> If I just ignore the nonNull parameter in HSAILMove.UncompressPointer, my test passes but of course
> the codegen is sub-optimal.
>
> -- Tom
>
>
>> -----Original Message-----
>> From: graal-dev [mailto:graal-dev-bounces at openjdk.java.net] On Behalf Of
>> Deneau, Tom
>> Sent: Monday, June 02, 2014 6:54 AM
>> To: Doug Simon
>> Cc: graal-dev at openjdk.java.net
>> Subject: RE: regression for compressed oops that require shift and add
>>
>> Doug --
>>
>> I will try to get some more details on this.
>>
>> -- Tom
>>
>>> -----Original Message-----
>>> From: Doug Simon [mailto:doug.simon at oracle.com]
>>> Sent: Monday, June 02, 2014 3:30 AM
>>> To: Deneau, Tom
>>> Cc: graal-dev at openjdk.java.net
>>> Subject: Re: regression for compressed oops that require shift and add
>>>
>>> The code appears to be correct for the AMD64 backend (i.e.
>>> UncompressPointer.emitCode()). But the HSAIL backend code looks
>>> plausibly correct as well. Do you know where the wrong code path is
>>> taken in the HSAIL backend while emitting code?
>>>
>>> -Doug
>>>
>>> On May 30, 2014, at 10:30 PM, Deneau, Tom <tom.deneau at amd.com> wrote:
>>>
>>>> All --
>>>>
>>>> I have noticed a regression when I update to the latest default tip.
>>>> This is in the area of compressed pointers.  We typically run with
>>> different heap sizes which
>>>> force different compression schemes.  The failure is in the scheme
>>> where the 32-bit compressed pointer
>>>> must be multiplied by 8 and then add a base,  we get this for
>>>> example
>>> with a max heap size of 31g.
>>>> The problem is that the shift and add must only be applied if the
>>> original compressed oop is not 0.
>>>> Previous codegen (good):
>>>> 	ld_global_s32 $s1, [$d1 + 16];
>>>> 	cvt_u64_u32 $d1, $s1;
>>>> 	cmp_eq_b1_u64 $c0, $d1, 0x0;
>>>> 	mad_u64 $d1, $d1, 8, 0x7fb447fff000;     // $d1 = $d1 * 8 + base
>>>> 	cmov_b64 $d1, $c0, 0x0, $d1;             // conditionally make
>>> $d1 0 if it was originally 0
>>>> Current codegen (bad);
>>>> 	ld_global_s32 $s1, [$d1 + 16];
>>>> 	cvt_u64_u32 $d1, $s1;
>>>> 	mad_u64 $d1, $d1, 8, 0x7f0137fff000;
>>>>
>>>> The other compression schemes seem to work ok.
>>>> I confess I had not merged with default for a few weeks so my
>>>> previous
>>> working version was based on default 8d0242a07f7e.  But I'm not sure
>>> where the regression happened in between there.
>>>> I guess you would not notice this in your gate unless you ran with a
>>> heap size big enough to cause this kind of compression.
>>>> -- Tom
>>>>