[aarch64-port-dev ] RFD: AOT for AArch64

[Andrew Dinn]

On 26/03/18 16:02, Andrew Dinn wrote:
> On 26/03/18 15:56, Andrew Dinn wrote:
>> Ship it!
> Ok, so I know this really needs a code audit too. I'm working on that now.
I am still trying to understand all the details of this patch so this is
really just preliminary feedback. Many of the comments below are
questions, posed mostly as requests for clarification rather than
suggestions for any improvement.

Also, I'm now switching to look at the Graal changes so I can tie these
two patches together.

regards,

Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander


One overall comment: copyrights need updating!

1) make/autoconf/generated-configure.sh

you can scratch all these changes as the file is now deleted

2)  make/hotspot/lib/JvmFeatures.gmk

is this a necessary part of the AOT change or just an extra cleanup?

3) src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

I believe this is the cause of the slowdebug assert . . .

@@ -738,25 +738,25 @@
-  if (far_branches() && !Compile::current()->in_scratch_emit_size()) {
+  if (UseAOT || (far_branches() &&
!Compile::current()->in_scratch_emit_size())) {
     address stub = emit_trampoline_stub(start_offset, entry.target());
     if (stub == NULL) {
       return NULL; // CodeCache is full
     }

 . . . and should be

@@ -738,25 +738,25 @@
-  if (far_branches() && !Compile::current()->in_scratch_emit_size()) {
+  if ((UseAOT || (far_branches()) &&
!Compile::current()->in_scratch_emit_size()) {
     address stub = emit_trampoline_stub(start_offset, entry.target());
     if (stub == NULL) {
       return NULL; // CodeCache is full
     }

Also, as mentioned earlier this next change is redundant as it is
already in the latest hs

@@ -1048,11 +1048,11 @@
                                Address::lsl(LogBytesPerWord)));
     ldr(method_result, Address(method_result, vtable_offset_in_bytes));
   } else {
     vtable_offset_in_bytes += vtable_index.as_constant() * wordSize;
     ldr(method_result,
-        form_address(rscratch1, recv_klass, vtable_offset_in_bytes));
+        form_address(rscratch1, recv_klass, vtable_offset_in_bytes, 0));


4) nativeInst_aarch64.hpp

 147 class NativePltCall: public NativeInstruction {
 148 public:
 149   enum Intel_specific_constants {

Hmm, Intel? Shurely AArch64?

5) src/hotspot/share/code/oopRecorder.hpp
   src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

Why is there a need to make these changes to shared code? In particular:

  why does allocate_metadata need to be virtual?

  what magic lies behind the argument 64 passed into the GrowableArray
created by AOTOopRecorder::AOTOopRecorder()?

6)
src/jdk.aot/share/classes/jdk.tools.jaotc.binformat/src/jdk/tools/jaotc/binformat/elf/ElfTargetInfo.java

         if (archStr.equals("amd64") || archStr.equals("x86_64")) {
             arch = Elf64_Ehdr.EM_X86_64;
+        } else if (archStr.equals("amd64") || archStr.equals("aarch64")) {
+            arch = Elf64_Ehdr.EM_AARCH64;

Should be

         if (archStr.equals("amd64") || archStr.equals("x86_64")) {
             arch = Elf64_Ehdr.EM_X86_64;
+        } else if (archStr.equals("aarch64")) {
+            arch = Elf64_Ehdr.EM_AARCH64;


7)
src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/AOTCompiledClass.java

     static String metadataName(HotSpotResolvedObjectType type) {
         AOTKlassData data = getAOTKlassData(type);
         assert data != null : "no data for " + type;
+        try {
+            AOTKlassData t = getAOTKlassData(type);
+            t.getMetadataName();
+        } catch (NullPointerException e) {
+            return type.getName();
+        }
         return getAOTKlassData(type).getMetadataName();
     }

This looks a bit like last night's left-overs? Why is
getAOTKlassData(type) being called again? Is this just dev-time
scaffolding you didn't delete? Or is the catch actually doing something
necessary?

8)
src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/CodeSectionProcessor.java

                     final Call callInfopoint = (Call) infopoint;
-                    if (callInfopoint.target instanceof
HotSpotForeignCallLinkage) {
+                    if (callInfopoint.target instanceof
HotSpotForeignCallLinkage &&
+                        target.arch instanceof AMD64) {
                         // TODO 4 is x86 size of relative displacement.

So, why can AArch64 simply not worry about zeroing out destinations? Is
this because of using PLTs and CompiledPltStaticCall::set_stub_to_clean?

9)
src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/CompiledMethodInfo.java

         for (Mark m : compilationResult.getMarks()) {
+            int adjOffset = m.pcOffset;
+            if (archStr.equals("aarch64")) {
+                // FIXME: This is very ugly.
+                // The mark is at the end of a group of three instructions:
+                // adrp; add; ldr
+                adjOffset += 12;
+            } else {
             // TODO: X64-specific code.

I'm not really sure why this is so ugly. Can you explain?

10) /jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/Linker.java

-        if (objFile.exists()) {
+        if (objFile.exists() &&
System.getenv("DO_NOT_DELETE_PRECIOUS_FILE") == null) {

Is this intended to remain?