RFR: 8169529: AArch64: Revert old JDK-8167595 changes after JDK-8159035 fix is pushed

[Rahul Raghavan]

Hi,

Please review updated webrev -
  http://cr.openjdk.java.net/~rraghavan/8169529/webrev.01/
hope new changes covers all earlier comments.

Thanks,
Rahul

> -----Original Message-----
> From: Rahul Raghavan
> Sent: Thursday, November 17, 2016 8:39 PM
> To: Andrew Dinn; Tobias Hartmann; Andrew Haley; aarch64-port-dev at openjdk.java.net
> Cc: Vladimir Kozlov; hotspot-compiler-dev at openjdk.java.net
> Subject: RE: RFR: 8169529: AArch64: Revert old JDK-8167595 changes after JDK-8159035 fix is pushed
> 
> Hi,
> 
> Sorry I missed that JDK-8159035 was Oracle closed bug.
> Thank you Tobias for forwarding the open changeset link
> and thanks for all review comments.
> I will check the pointers from Andrew Dinn and will submit revised webrev for review.
> 
> - Rahul
> 
> > -----Original Message-----
> > From: Andrew Dinn [mailto:adinn at redhat.com]
> > Sent: Thursday, November 17, 2016 6:14 PM
> > To: Tobias Hartmann; Andrew Haley; Rahul Raghavan; ningsheng.jian at linaro.org; Vladimir Kozlov; aarch64-port-
> > dev at openjdk.java.net
> > Cc: hotspot-compiler-dev at openjdk.java.net
> > Subject: Re: RFR: 8169529: AArch64: Revert old JDK-8167595 changes after JDK-8159035 fix is pushed
> >
> > On 17/11/16 11:57, Tobias Hartmann wrote:
> > > this looks good to me. Here are the changes for JDK-8159035 (should have been reviewed in the open):
> > > http://hg.openjdk.java.net/jdk9/hs/jdk/rev/0e98c765ce9b
> >
> > Thanks, Tobias.
> >
> > I understand that a zero check is no longer needed but I am not sure it
> > is correct to revert the AArch64 change. For example, at line 2820+ the
> > following change is being backed out
> >
> > -      __ subw(len_reg, len_reg, 16);
> > -      __ cbnzw(len_reg, L_aes_loop);
> > +      __ sub(len_reg, len_reg, 16);
> > +      __ cbnz(len_reg, L_aes_loop);
> >
> > I think the use of subw and cbnzw was one of Andrew's mods to the
> > originally proposed patch. It ensures that the contents of len_reg are
> > correctly treated as a 32-bit int. Similarly, the proposed removal of
> > the zero test at line 2759 once again risks treating the 32 bit length
> > as a 64 bit quantity
> >
> > -      __ subsw(rscratch2, len_reg, zr);
> > -      __ br(Assembler::LE, _);
> > +      __ mov(rscratch2, len_reg);
> >
> > I think that should revert to a movw (of course this still makes the
> > change to add label L_finish redundant).
> >
> > So, rather than completely /revert/ the change I think it needs
> > /correcting/ just to remove the zero check.
> >
> > regards,
> >
> >
> > Andrew Dinn
> > -----------
> > Senior Principal Software Engineer
> > Red Hat UK Ltd
> > Registered in England and Wales under Company Registration No. 03798903
> > Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander