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

[Andrew Hughes]

On 09:43 Fri 27 Nov     , Aleksey Shipilev wrote:
> On 11/27/20 8:33 AM, Andrew Hughes wrote:
> > On 07:21 Fri 27 Nov     , Andrew Hughes wrote:
> > > Umbrella Bug: https://bugs.openjdk.java.net/browse/JDK-8257192
> > > Webrevs:
> > >    jdk: https://cr.openjdk.java.net/~andrew/openjdk8/8257192/jdk/webrev.01/
> 
> *) I don't quite understand this change in
> test/jdk/jfr/event/os/TestCPUInformation.java: it seems to replace zArch
> with s390? Seems unrelated to AArch64 port?
> 
> -            Events.assertField(event, "description").containsAny("Intel",
> "AMD", "Unknown x86", "SPARC", "ARM", "PPC", "PowerPC", "AArch64", "zArch");
> +            Events.assertField(event, "description").containsAny("Intel",
> "AMD", "Unknown x86", "SPARC", "ARM", "PPC", "PowerPC", "AArch64", "s390");
> 
> Otherwise looks fine.

I was expecting this, as I wondered the same myself. It's part of JDK-8215961:

8215961: jdk/jfr/event/os/TestCPUInformation.java fails on AArch64

In 11u, there is an S390 port as well, so it actually fixes both AArch64 & s390.
Obviously, we don't have src/hotspot/cpu/s390/vm_version_ext_s390.cpp in 8u, but
I see no harm in including the test case fix. S390 is Zero only on 8u anyway, so
no-one on that platform is going to have JFR.

> 
> > >    hotspot: https://cr.openjdk.java.net/~andrew/openjdk8/8257192/hotspot/webrev.01/
> 
> *) agent/src/share/classes/sun/jvm/hotspot/HSDB.java:
>   - it seems to change semantics a bit by removing ia64 block. Shouldn't it just add the aarch64 test?
>

Done.

> *) agent/src/share/classes/sun/jvm/hotspot/utilities/PlatformInfo.java
>  - weird if-elsing:
> 
>   66     } else {if (cpu.equals("aarch64")) {
>   67       return cpu;
>   68     } else
> 
>   ...it should be:
> 
>   66     } else if (cpu.equals("aarch64")) {
>   67       return cpu;
>   68     } else {
>

Done.

> *) src/os/linux/vm/os_linux.cpp:
>   - SYS_getcpu handling, wouldn't it be easier to just add "||
> defined(AARCH64)" to existing code shape, instead of moving the block
> around?

I think the changed version is better as they share:

retval = syscall(SYS_getcpu, &cpu, NULL, NULL);

and that makes it clear.

>   - it also looks like this line:
> 
>    2964 #  define SYS_getcpu AARCH64_ONLY(168) NOT_AARCH64(318)
> 
>   ...is cleaner to write as:
> 
>    2964 #  define SYS_getcpu AARCH64_ONLY(168) IA32_ONLY(318)
>

Done, that better matches the other cases in that file.

> 
> *) 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.

> *) src/share/vm/c1/c1_LIR.cpp and .hpp:
>   - why "defined (TARGET_ARCH_aarch64)", and not "defined (AARCH64)"? We
> seem to use both forms in these files.
>

I suspect TARGET_ARCH_aarch64 is a legacy of the simulator where AARCH64 may
not have been defined. Fixed to use defined (AARCH64) consistently, as the
other architectures in there don't use TARGET_ARCH.

> *) src/share/vm/c1/c1_LIRAssembler.cpp:
>   - stray space before "ifndef":
> 
>   173 # ifndef AARCH64
>

Fixed.

> *) src/share/vm/c1/c1_LIRGenerator.cpp:
>   - this seems to include the CMS pre-cleaning bugfix (JDK-8135018), which
> is technically in shared code. But I think this is acceptable.
> 
> *) src/share/vm/c1/c1_LinearScan.cpp:
>   - ifndef-ing ShouldNotReachHere is a bit odd.
>   - in any case, other hunks in this file convert "#ifdef X" to "#if
> defined(X)", so this change should really be "#if !defined(AARCH64)" then?
>

Fixed the format, but I'd rather leave any changes in semantics to a separate fix,
if necessary.

> *) src/share/vm/c1/c1_Runtime1.cpp:
>  - why add "break" to "default"?

At one point, the assert was commented out. I guess this can go now.
The block is in a #ifdef ASSERT block.

>  - a new line additions at L222, L1139 is superfluous?
>

Fixed.

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

> 
> *) src/share/vm/code/compiledIC.hpp
>   - I believe the appropriate style is with parentheses: "#if defined(AARCH64) && !defined(ZERO)"
>

Done

> *) src/share/vm/memory/allocation.inline.hpp:
>   - Update comment: ...and AARCH64 now?
>
>   40 #if defined(SPARC) || defined(X86) || defined(AARCH64)
>   41   // Sparc and X86 have atomic jlong (8 bytes) instructions
>

Done


> *) src/share/vm/opto/graphKit.cpp:
>   - I am a bit nervous about changing the default MemOrd for all GCs. I
> understand that is the part of a large "8064611: AARCH64: Changes to HotSpot
> shared code" integration. Shouldn't it be protected with AARCH64_ONLY as
> well, as other shared VM changes are in this patch?
>

Done

> *) src/share/vm/opto/library_call.cpp
>    src/share/vm/opto/memnode.cpp:
>   - Ugh. This touches a rather delicate piece of code. I'd like Andrews
> (Haley and/or Dinn) explicitly sign off on this.
>

All these changes are already reviewed and in the AArch64 tree.

> *) src/share/vm/opto/macro.cpp:
>   - This change is quite messy:
> 
>   1388     if ( AARCH64_ONLY ( !alloc->does_not_escape_thread() &&
>   1389                         (init == NULL ||
>   1390                          !init->is_complete_with_arraycopy()) )
>   1391          NOT_AARCH64  ( init == NULL ||
>   1392                         (!init->is_complete_with_arraycopy() &&
>   1393                          !init->does_not_escape()) )
>   1394        ) {
> 
>   I understand it protects JDK-8136596 patch, which did:
> 
>   -    if (init == NULL || (!init->is_complete_with_arraycopy() && !init->does_not_escape())) {
>   +    if (!alloc->does_not_escape_thread() &&
>   +        (init == NULL || !init->is_complete_with_arraycopy())) {
> 
>   So I'd suggest to make it clearer:
> 
>   #ifndef AARCH64
>     if (init == NULL || (!init->is_complete_with_arraycopy() && !init->does_not_escape())) {
>   #else
>     if (!alloc->does_not_escape_thread() &&
>         (init == NULL || !init->is_complete_with_arraycopy())) {
>   #endif

Done

> 
> *) src/share/vm/prims/jvmtiTagMap.cpp
>  - Why "ifdef ASSERT"?
>

The method is only called from an assert, but this change to have crept in and is unrelated to AArch64,
so I've reverted it.

It was part of https://icedtea.classpath.org/hg/icedtea8-forest/hotspot/rev/a8b6c290873cc30d9 and the
other changes have been removed.

> *) src/share/vm/runtime/arguments.cpp
>  - I'd prefer a cleaner:
> 
>   #ifndef AARCH64
>     FLAG_SET_DEFAULT(ReservedCodeCacheSize, ReservedCodeCacheSize * 5)
>   #else
>     FLAG_SET_DEFAULT(ReservedCodeCacheSize,
>                      MIN2(CODE_CACHE_DEFAULT_LIMIT, ReservedCodeCacheSize * 5))
>   #endif
>

Done.

New HotSpot webrev:

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

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