Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
On Thu, 24 Sep 2020 04:52:22 GMT, David Holmes wrote: >> Monica Beckwith has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update orderAccess_windows_aarch64.hpp >> >> changing from Acq-reL to Sequential Consistency to avoid compiler >> reordering when no ordering hints are provided > > src/hotspot/share/runtime/stubRoutines.cpp line 397: > >> 395: // test safefetch routines >> 396: // Not on Windows 32bit until 8074860 is fixed >> 397: #if ! (defined(_WIN32) && defined(_M_IX86)) && !defined(_M_ARM64) > > The comment needs updating to refer to Aarch64. This is actually not needed anymore, as it does work just fine on Windows+Aarch64. Presumably we have added it in the early days of the port when we haven't figured out exception handling quite yet. Thanks for catching David. - PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
On Thu, 24 Sep 2020 04:45:16 GMT, David Holmes wrote: >> Monica Beckwith has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update orderAccess_windows_aarch64.hpp >> >> changing from Acq-reL to Sequential Consistency to avoid compiler >> reordering when no ordering hints are provided > > src/hotspot/os/windows/os_windows.cpp line 2546: > >> 2544: >> 2545: #ifdef _M_ARM64 >> 2546: // Unsafe memory access > > I'm not at all clear why this unsafe memory access handling is for Aarch64 > only? Hum, this was already part of [JDK-8250810](https://github.com/openjdk/jdk/commit/a4eaf9536c272862b5ec856bf263679be09bddc9) / [JDK-8248817](https://github.com/openjdk/jdk/commit/257809d7440e87ac595d03b6c9a98d8f457f314c) (see a couple lines below in this PR) without the `ifdef` for Aarch64, but I messed up rebasing on top of it. I removed it now, thanks for catching! - PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
On Thu, 24 Sep 2020 04:57:39 GMT, David Holmes wrote: >> Monica Beckwith has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update orderAccess_windows_aarch64.hpp >> >> changing from Acq-reL to Sequential Consistency to avoid compiler >> reordering when no ordering hints are provided > > I've mainly looked at the non-Aarch64 specific changes. A couple of minor > comments below. @mo-beck This PR should not be associated with any JBS issues which are targeted at repo-aarch64-port. All such issues should have been closed by now when the associated code was pushed to aarch64-port repo. That was the whole point of creating separate issues so that they could be tracked in the porting repo. Now that all of that work should be complete the only issue that should remain open in relation to the Windows-Aarch64 port is JDK-8248238. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
On Wed, 23 Sep 2020 13:23:43 GMT, 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] >> * The define-guards around R18 have been changed to `R18_RESERVED`. This >> will be enabled for Windows only for now but >> will be required for the upcoming macOS+Aarch64 [2] port as well. >> * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov >> in our PR for now and built the >> Windows-specific CPU feature detection on top of it. >> >> [1] >> https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html >> [2] https://openjdk.java.net/jeps/8251280 > > Monica Beckwith has updated the pull request incrementally with one > additional commit since the last revision: > > Update orderAccess_windows_aarch64.hpp > > changing from Acq-reL to Sequential Consistency to avoid compiler > reordering when no ordering hints are provided I've mainly looked at the non-Aarch64 specific changes. A couple of minor comments below. src/hotspot/os/windows/os_windows.cpp line 2546: > 2544: > 2545: #ifdef _M_ARM64 > 2546: // Unsafe memory access I'm not at all clear why this unsafe memory access handling is for Aarch64 only? src/hotspot/share/runtime/stubRoutines.cpp line 397: > 395: // test safefetch routines > 396: // Not on Windows 32bit until 8074860 is fixed > 397: #if ! (defined(_WIN32) && defined(_M_IX86)) && !defined(_M_ARM64) The comment needs updating to refer to Aarch64. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
> 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] > * The define-guards around R18 have been changed to `R18_RESERVED`. This will > be enabled for Windows only for now but > will be required for the upcoming macOS+Aarch64 [2] port as well. > * We've incorporated https://github.com/openjdk/jdk/pull/154 by @AntonKozlov > in our PR for now and built the > Windows-specific CPU feature detection on top of it. > > [1] > https://mail.openjdk.java.net/pipermail/aarch64-port-dev/2020-August/009597.html > [2] https://openjdk.java.net/jeps/8251280 Monica Beckwith has updated the pull request incrementally with one additional commit since the last revision: Update orderAccess_windows_aarch64.hpp changing from Acq-reL to Sequential Consistency to avoid compiler reordering when no ordering hints are provided - Changes: - all: https://git.openjdk.java.net/jdk/pull/212/files - new: https://git.openjdk.java.net/jdk/pull/212/files/50ab8edf..4da7b89e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=212=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=212=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/212.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/212/head:pull/212 PR: https://git.openjdk.java.net/jdk/pull/212
Re: RFR: 8248238: Implementation of JEP: Windows AArch64 Support [v5]
On Mon, 21 Sep 2020 14:47:15 GMT, Bernhard Urban-Forster
wrote:
>>> _Mailing list message from [Andrew Haley](mailto:a...@redhat.com) on
>>> [build-dev](mailto:build-dev@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:a...@redhat.com) on
>> [build-dev](mailto:build-dev@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:a...@redhat.com) on
> [build-dev](mailto:build-dev@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://keybase.io/andrewhaley
>
