[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