[EXT] Re: [aarch64-port-dev ] RFR(XXS): 8222412: AARCH64: lse atomics encoding is not accepting zr as source

[Derek White]

Hi Andrew,

I asked Dmitrij to look at instruction encodings. 

The instruction encodings are the basis to the back-end of the aarch64 port. There have been at least 10 latent bugs [1] reported in these encodings. These were unimportant because they weren't used, until they were.

These encoding bugs take little effort to find, less to review, and are a small investment to make future changes in the aarch64 port easier. The effort is much lower to find and fix them at once, instead of piecemeal over a few years. Furthermore, this isn't an open universe of issues - there is a well-defined, fix set of instructions to check. 

We've appreciated your detailed and thoughtful reviews immensely, and I might agree that these bugs aren't worth *your* time reviewing. But I think some of your concerns reflect a lack of review capacity in the aarch64 port. The rest of the JDK happily accepts checkins that are nothing more than fixing punctuation in code comments.

In the meantime, we can batch up the encoding bugs in a bundle or two. 

Thanks again for your reviews and all your contributions to the aarch64 port,
 - Derek

[1] https://bugs.openjdk.java.net/browse/JDK-8210578?jql=text%20~%20%22encoding%20aarch64%22

> -----Original Message-----
> From: aarch64-port-dev <aarch64-port-dev-bounces at openjdk.java.net> On
> Behalf Of Andrew Dinn
> Sent: Monday, April 15, 2019 12:16 PM
> To: Dmitrij Pochepko <dmitrij.pochepko at bell-sw.com>; Andrew Haley
> <aph at redhat.com>; aarch64-port-dev at openjdk.java.net; hotspot compiler
> <hotspot-compiler-dev at openjdk.java.net>
> Subject: [EXT] Re: [aarch64-port-dev ] RFR(XXS): 8222412: AARCH64: lse
> atomics encoding is not accepting zr as source
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hello Dmitrij,
> 
> On 12/04/2019 16:24, Dmitrij Pochepko wrote:
> > please review small fix for 8222412: AARCH64: lse atomics encoding is
> > not accepting zr as source
> >
> > webrev: http://cr.openjdk.java.net/~dpochepk/8222412/webrev.01/
> >
> > Current encoding for lse atomics hits assert when trying to use zr as
> > source register while it is allowed by spec. Current vm doesn't use
> > atomics with zr and this problem is not triggered.
> 
> I think this part of your comment is critical:
> 
>   "Current vm doesn't use atomics with zr and this problem is not triggered."
> 
> In which case I have to ask why are you spending time fixing this and asking
> others to spend time reviewing it? I agree that this detail is indeed wrong. In
> related news the whole internet is broken yet that's no reason for anyone to
> spend their time trying to fix it. Do you have a use case for this fixed
> behaviour?
> 
> That comment may sound harsh but this fix is the nadir (at least I hope it is)
> of a trajectory that has presented change after change offering little by way
> of motivation and, in direct consequence, little by way of benefit. It is all
> very nice to receive contributions gratis but they really need to be worth
> more than the cost of accepting them.
> 
> > Testing:
> >
> > I generated lse atomics with zr as source register. No assert observed
> > with patched vm.
> 
> I'd much prefer for this to be tested through being used in the VM.
> Perhaps you might re-present the patch as part of a larger patch that justifies
> its inclusion by fixing an actual breakage or performance problem. If you can
> do that I'd be happy to pass it as reviewed.
> 
> > CR: https://bugs.openjdk.java.net/browse/JDK-8222412
> 
> Also, could you please downgrade the priority of this defect to P5 so it
> represent the true state of affairs.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------