RFD: AOT for AArch64 (original) (raw)

Andrew Haley aph at redhat.com
Tue Mar 27 08:54:20 UTC 2018

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/macroAssembleraarch64.cpp

I believe this is the cause of the slowdebug assert . . . @@ -738,25 +738,25 @@ - if (farbranches() && !Compile::current()->inscratchemitsize()) { + if (UseAOT || (farbranches() && !Compile::current()->inscratchemitsize())) { address stub = emittrampolinestub(startoffset, entry.target()); if (stub == NULL) { return NULL; // CodeCache is full } . . . and should be @@ -738,25 +738,25 @@ - if (farbranches() && !Compile::current()->inscratchemitsize()) { + if ((UseAOT || (farbranches()) && !Compile::current()->inscratchemitsize()) { address stub = emittrampolinestub(startoffset, 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(methodresult, Address(methodresult, vtableoffsetinbytes)); } else { vtableoffsetinbytes += vtableindex.asconstant() * wordSize; ldr(methodresult, - formaddress(rscratch1, recvklass, vtableoffsetinbytes)); + formaddress(rscratch1, recvklass, vtableoffsetinbytes, 0));

Right.

4) nativeInstaarch64.hpp

147 class NativePltCall: public NativeInstruction { 148 public: 149 enum Intelspecificconstants { 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 allocatemetadata 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("x8664")) { arch = Elf64Ehdr.EMX8664; + } else if (archStr.equals("amd64") || archStr.equals("aarch64")) { + arch = Elf64Ehdr.EMAARCH64; Should be if (archStr.equals("amd64") || archStr.equals("x8664")) { arch = Elf64Ehdr.EMX8664; + } else if (archStr.equals("aarch64")) { + arch = Elf64Ehdr.EMAARCH64;

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

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("DONOTDELETEPRECIOUSFILE") == 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

More information about the hotspot-dev mailing list