RFD: AOT for AArch64 (original) (raw)

Andrew Dinn adinn at redhat.com
Tue Mar 27 08🔞48 UTC 2018

• Previous message: RFD: AOT for AArch64
• Next message: RFD: AOT for AArch64
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

1. make/hotspot/lib/JvmFeatures.gmk

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

1. 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));

1. nativeInst_aarch64.hpp

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

Hmm, Intel? Shurely AArch64?

1. 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()?

1. 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;

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

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

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

1. /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?

• Previous message: RFD: AOT for AArch64
• Next message: RFD: AOT for AArch64
• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the hotspot-dev mailing list