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

[Aleksey Shipilev]

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.

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

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

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


*) src/share/vm/adlc/formssel.cpp:
   - equivalent_predicates addition changes the shared code path. Is this a bugfix?

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

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

   173 # ifndef AARCH64

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

*) src/share/vm/c1/c1_Runtime1.cpp:
  - why add "break" to "default"?
  - a new line additions at L222, L1139 is superfluous?

*) src/share/vm/c1/c1_Runtime1.hpp
  - so, "move_klass_patching" is undefined for AARCH64 (see .cpp change), should it be undeclared too?

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

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

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

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

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

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

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


> https://cr.openjdk.java.net/~andrew/openjdk8/8257192/root/webrev.01/

Looks fine.


-- 
Thanks,
-Aleksey