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

Vladimir Kozlov vladimir.kozlov at oracle.com
Tue Mar 27 14:20:31 UTC 2018


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

How about aot.keep.objFile?

jaotc --output libtest.so -J-Daot.keep.objFile=true test.class

And I found that on Linux we forgot to add '.o' suffix.

diff -r 898ef81cbc0e 
src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/Linker.java
--- 
a/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/Linker.java
+++ 
b/src/jdk.aot/share/classes/jdk.tools.jaotc/src/jdk/tools/jaotc/Linker.java
@@ -69,6 +69,7 @@
                  if (name.endsWith(".so")) {
                      objectFileName = name.substring(0, name.length() - 
".so".length());
                  }
+                objectFileName = objectFileName + ".o";
                  linkerPath = (options.linkerpath != null) ? 
options.linkerpath : "ld";
                  linkerCmd = linkerPath + " -shared -z noexecstack -o " 
+ libraryFileName + " " + objectFileName;
                  linkerCheck = linkerPath + " -v";
@@ -130,7 +131,8 @@
              throw new InternalError(errorMessage);
          }
          File objFile = new File(objectFileName);
-        if (objFile.exists()) {
+        boolean keepObjFile = 
Boolean.parseBoolean(System.getProperty("aot.keep.objFile", "false"));
+        if (objFile.exists() && !keepObjFile) {
              if (!objFile.delete()) {
                  throw new InternalError("Failed to delete " + 
objectFileName + " file");
              }

Thanks,
Vladimir

On 3/27/18 1:54 AM, Andrew Haley wrote:
> 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.
> 


More information about the aarch64-port-dev mailing list