regression for compressed oops that require shift and add

[Deneau, Tom]

Do you have tests that explicitly pass null objects in?
By the way, how do you control the heap size from your junits?

-- Tom

> -----Original Message-----
> From: graal-dev [mailto:graal-dev-bounces at openjdk.java.net] On Behalf Of
> Roland Schatz
> Sent: Tuesday, June 03, 2014 4:04 AM
> To: graal-dev at openjdk.java.net
> Subject: Re: regression for compressed oops that require shift and add
> 
> 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
> >>>>