[8u] RFR: JDK-8257192: Integrate AArch64 JIT port into 8u

[Andrew Hughes]

On 14:45 Mon 30 Nov     , Aleksey Shipilev wrote:
> Hi,
> 
> Thanks for explanations. These are the only things left from my review:
> 
> On 11/27/20 8:23 PM, Andrew Hughes wrote:
> > > *) src/share/vm/adlc/formssel.cpp:
> > >    - equivalent_predicates addition changes the shared code path. Is this a bugfix?
> > > 
> > 
> > 8145438: Guarantee failures since 8144028: Use AArch64 bit-test instructions in C2
> > 
> > Made AARCH64_ONLY.
> 
> You can write this:
> 
> 1246       _matrule->equivalent(AD.globalNames(), short_branch->_matrule) &&
> 1247       AARCH64_ONLY(equivalent_predicates(this, short_branch)) NOT_AARCH64(true)) {
> 
> as:
> 
> 1246       _matrule->equivalent(AD.globalNames(), short_branch->_matrule)
> 1247       AARCH64_ONLY(&& equivalent_predicates(this, short_branch))) {
> 
>

I was in two minds between this and wanting to keep the symmetry of the '&&' on the line
above. Neither is perfect, but I've switched to this one now.

> > > *) src/share/vm/c1/c1_Runtime1.hpp
> > >   - so, "move_klass_patching" is undefined for AARCH64 (see .cpp change), should it be undeclared too?
> > 
> > It's in src/cpu/aarch64/vm/c1_Runtime1_aarch64.cpp
> 
> Yes, but under TARGET_ARCH_aarch64, *_patching and friends remains _only_
> declared, not defined. Shouldn't the entire *_patching declaration block be
> e.g. this?
> 
>  #ifdef TARGET_ARCH_aarch64
>    static void patch_code_aarch64(JavaThread* thread, StubID stub_id);
>  #else
>    static int access_field_patching(JavaThread* thread);
>    static int move_klass_patching(JavaThread* thread);
>    static int move_mirror_patching(JavaThread* thread);
>    static int move_appendix_patching(JavaThread* thread);
>    static void patch_code(JavaThread* thread, StubID stub_id);
>  #endif
>

Still not sure what you're aiming at here. The routines are defined in
c1_Runtime1_aarch64.cpp. If they were not defined anywhere, surely
the build would fail? Given how well tested the current version is,
I'd prefer to leave it as is in the current patch, so as to not to
deviate too far in the initial import or delay this any
further. You're welcome to change this in a follow-up patch.

> > https://cr.openjdk.java.net/~andrew/openjdk8/8257192/hotspot/webrev.02/
> 
> I have only a philosophical point left: if we push this to 8u282, this means
> we start the clock on getting any remaining issues resolved in December
> before the January release. Since December is the time when a significant
> part of involved people take vacations, I think we are risking this quite a
> bit. While not exactly the concern of OpenJDK 8u Upstream, it would also
> mean we would need to merge this back to aarch64-port/jdk8u-shenandoah, and
> make sure that Shenandoah itself is not broken.
> 
> It would make me sleep and relax better if we integrate it first thing in
> 8u292 (for April CPU), so that we would have plenty of time next year to
> work out the kinks, if any. This is ultimately something for 8u Maintainers
> to decide, of course.
>

I think breakage is unlikely - and that's why I'm trying to minimise differences
from what's already in aarch64/shenandoah-jdk8u where possible - but we're now
aiming for 8u292.

https://cr.openjdk.java.net/~andrew/openjdk8/8257192/hotspot/webrev.03/

has the first change listed above and also JDK-8221725, following the inclusion
of JDK-8221408 in 8u292.

Differences look like this:

$ diff -u ../../webrevs/openjdk8/8257192/hotspot/webrev.02/hotspot.patch ../../webrevs/openjdk8/8257192/hotspot/webrev.03/hotspot.aarch64.patch |egrep '^[+-][+-] ' 
-+      _matrule->equivalent(AD.globalNames(), short_branch->_matrule) &&
-+      AARCH64_ONLY(equivalent_predicates(this, short_branch)) NOT_AARCH64(true)) {
++      _matrule->equivalent(AD.globalNames(), short_branch->_matrule)
++      AARCH64_ONLY(&& equivalent_predicates(this, short_branch))) {
-+    __ mov(tmp, (address) (~(os::vm_page_size()-1) | markOopDesc::lock_mask_in_place));
++    __ mov(tmp, (address) (~(os::vm_page_size()-1) | (uintptr_t)markOopDesc::lock_mask_in_place));

Doing a test build with this now across all architectures. Let's finally get this in.

> -- 
> Thanks,
> -Aleksey
> 

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
OpenJDK Package Owner
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222