RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]

[Monica Beckwith]

On Mon, 21 Sep 2020 14:47:15 GMT, Bernhard Urban-Forster <burban at openjdk.org> wrote:

>>> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on [build-dev](mailto:build-dev at openjdk.java.net):_
>>> 
>>> On 18/09/2020 11:14, Monica Beckwith wrote:
>>> 
>>> > This is a continuation of https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html
>>> 
>>> The diffs in assembler_aarch64.cpp are mostly spurious. Please try this.
>> 
>> 
>> Thank you Andrew. Is the goal to reduce the patch diff in `assembler_aarch64.cpp`? If so, we need to get rid of the
>> retry in your patch to avoid additional calls to `random`, e.g. something like this (the diff for the generated part
>> would look indeed nicer with that:  https://gist.github.com/lewurm/2e7b0e00447696c75e00febb83034ba1 ):
>> --- a/src/hotspot/cpu/aarch64/aarch64-asmtest.py
>> +++ b/src/hotspot/cpu/aarch64/aarch64-asmtest.py
>> @@ -13,6 +13,8 @@ class Register(Operand):
>> 
>>      def generate(self):
>>          self.number = random.randint(0, 30)
>> +        if self.number == 18:
>> +            self.number = 17
>>          return self
>> 
>>      def astr(self, prefix):
>> @@ -37,6 +39,8 @@ class GeneralRegisterOrZr(Register):
>> 
>>      def generate(self):
>>          self.number = random.randint(0, 31)
>> +        if self.number == 18:
>> +            self.number = 16
>>          return self
>> 
>>      def astr(self, prefix = ""):
>> @@ -54,6 +58,8 @@ class GeneralRegisterOrZr(Register):
>>  class GeneralRegisterOrSp(Register):
>>      def generate(self):
>>          self.number = random.randint(0, 31)
>> +        if self.number == 18:
>> +            self.number = 15
>>          return self
>> 
>>      def astr(self, prefix = ""):
>> @@ -1331,7 +1337,7 @@ generate(SpecialCases, [["ccmn",   "__ ccmn(zr, zr, 3u, Assembler::LE);",
>>                          ["st1w",   "__ sve_st1w(z0, __ S, p1, Address(r0, 7));",         "st1w\t{z0.s}, p1, [x0, #7, MUL VL]"],
>>                          ["st1b",   "__ sve_st1b(z0, __ B, p2, Address(sp, r1));",        "st1b\t{z0.b}, p2, [sp, x1]"],
>>                          ["st1h",   "__ sve_st1h(z0, __ H, p3, Address(sp, r8));",        "st1h\t{z0.h}, p3, [sp, x8, LSL #1]"],
>> -                        ["st1d",   "__ sve_st1d(z0, __ D, p4, Address(r0, r18));",       "st1d\t{z0.d}, p4, [x0, x18, LSL #3]"],
>> +                        ["st1d",   "__ sve_st1d(z0, __ D, p4, Address(r0, r17));",       "st1d\t{z0.d}, p4, [x0, x17,
>> LSL #3]"],
>>                          ["ldr",    "__ sve_ldr(z0, Address(sp));",                       "ldr\tz0, [sp]"],
>>                          ["ldr",    "__ sve_ldr(z31, Address(sp, -256));",                "ldr\tz31, [sp, #-256, MUL VL]"],
>>                          ["str",    "__ sve_str(z8, Address(r8, 255));",                  "str\tz8, [x8, #255, MUL VL]"],
>
>> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on [build-dev](mailto:build-dev at openjdk.java.net):_
>> 
>> On 21/09/2020 09:18, Bernhard Urban-Forster wrote:
>> 
>> > Thank you Andrew. Is the goal to reduce the patch diff in `assembler_aarch64.cpp`? If so, we need to get rid of the
>> > retry in your patch to avoid additional calls to `random`, e.g. something like this (the diff for the generated part
>> > would look indeed nicer with that:  https://gist.github.com/lewurm/2e7b0e00447696c75e00febb83034ba1 ):
>> > [...]
>> 
>> Yes, better. Thanks.
> 
> Great, I've updated the PR. Thank you!

Thanks, Andrew for catching that. I have made the needful changes.

-Monica

> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on [build-dev](mailto:build-dev at openjdk.java.net):_
> 
> On 18/09/2020 11:14, Monica Beckwith wrote:
> 
> > This is a continuation of
> > https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009566.html
> > Changes since then:
> > * We've improved the write barrier as suggested by Andrew [1]
> 
> It's still wrong, I'm afraid. This is not a full barrier:
> 
> +#define FULL_MEM_BARRIER atomic_thread_fence(std::memory_order_acq_rel);
> 
> it is only StoreStore|LoadStore|LoadLoad, but you need StoreLoad as
> well. It might well be that you get the same DMB ISH instruction, but
> unless you use a StoreLoad barrier here it's theoretically possible
> for a compiler to reorder accesses so that a processor sees its own
> stores before other processors do. (And it's confusing for the reader
> too.)
> 
> Use:
> 
> +#define FULL_MEM_BARRIER atomic_thread_fence(std::memory_order_seq_cst);
> 
> See here:
> 
> https://en.cppreference.com/w/cpp/atomic/memory_order
> 
> memory_order_seq_cst "...plus a single total order exists in which all
> threads observe all modifications in the same order (see
> Sequentially-consistent ordering below)"
> 
> --
> Andrew Haley (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

-------------

PR: https://git.openjdk.java.net/jdk/pull/212