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

[Andrew Hughes]

On 17:15 Wed 27 Jan     , Aleksey Shipilev wrote:
> On 1/25/21 8:43 PM, Andrew Hughes wrote:
> > > > > *) 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.
> 
> Yeah, it is not a build breakage to define the method and not implement it,
> as long as nobody tries to reference the method. When that happens, linker
> would complain. But I agree about leaving it as is for integration sanity.
>

Yeah, I'm not currently convinced they are all unused and, unlike the
other changes, it is a significant code change, rather than cleanup
and rewriting the same thing in a better way. Definitely something that
warrants its own bug and review IMHO.

> > 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.
> 
> Okay, this looks good to me.
> 
> We are (hopefully) done!
>

Great, thanks! I've flagged the metabug for approval.

> -- 
> Thanks,
> -Aleksey
> 
-- 
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