[aarch64-port-dev ] RFD: AOT for AArch64
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/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
More information about the aarch64-port-dev
mailing list