RFD: AOT for AArch64

[Andrew Haley]

On 03/27/2018 09:18 AM, Andrew Dinn wrote:
> 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.

Cool.

> One overall comment: copyrights need updating!

I really need some way to automate that.  :-)

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

It's a hangover from debug code.

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

Yep, got that.

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

Right.

> 4) nativeInst_aarch64.hpp
> 
>  147 class NativePltCall: public NativeInstruction {
>  148 public:
>  149   enum Intel_specific_constants {
> 
> Hmm, Intel? Shurely AArch64?

LOL!  There's still some cruft in there.

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

That's another hangover.

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

That too.

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

Good catch!  I didn't see that one.

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

I'll dig.

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

It's not necessary on AARch64.  There's no need to do an AArch64 version of
this code.

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

Oh, it just sucks to have CPU-specific code there.  I guess I was in a bad
mood.

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

Yes.  Vladimir suggested we use a property for this, and I'm hoping someone will
come up with a suggested name.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671