RFR(XXS): 8222412: AARCH64: lse atomics encoding is not accepting zr as source

[Andrew Dinn]

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