-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AArch64: Add support for AArch64 in JIT-related files #4447
Conversation
1fda13d
to
734e7bd
Compare
8e00cc4
to
49c0f3d
Compare
Corrected copyright year. |
The change in runtime/compiler/module.xml suppresses building the JIT for the time being. |
Added changes in jilconsts.c. |
runtime/codert_vm/cnathelp.cpp
Outdated
@@ -247,6 +247,13 @@ J9_EXTERN_BUILDER_SYMBOL(icallVMprJavaSendStatic1); | |||
#define JIT_LO_U64_RETURN_REGISTER gpr.numbered[0] | |||
#define JIT_HI_U64_RETURN_REGISTER gpr.numbered[1] | |||
|
|||
#elif defined(J9VM_ARCH_AARCH64) | |||
|
|||
#define JIT_STACK_OVERFLOW_SIZE_REGISTER gpr.numbered[18] // To be decided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add TODO
so the comment is // TODO: To be decided
as it's easier to see that the register matching isn't complete?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the comment.
@@ -23,6 +23,7 @@ SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-excepti | |||
<module> | |||
<artifact type="target" name="j9jitlauncher"> | |||
<include-if condition="spec.flags.interp_nativeSupport"/> | |||
<exclude-if condition="spec.linux_aarch64.*" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removed temporarily? If so, can you add a comment using <!-- TODO: Issue #### disable for .... -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is only temporary. I need the line until the JIT for aarch64 can be built.
I am going to add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tag the TODO against an issue that will be closed when it's resolved? Other wise it's difficult to determine when a TODO in the code has been resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened Issue #4854 for this.
@@ -1746,6 +1746,8 @@ void jitAddSpilledRegisters(J9StackWalkState * walkState, void * stackMap) | |||
} | |||
while (savedGPRs != 0); | |||
} | |||
#elif defined(TR_HOST_ARM64) | |||
// To be filled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a #error
here until this is ready? Or if you need it to compile, a Trace Assert so the code will assert at runtime? It's much easier to find the right places to patch if we explicitly mark them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need it for compiling, and I am going to add assert.
runtime/oti/JITInterface.hpp
Outdated
@@ -177,6 +186,8 @@ class VM_JITInterface | |||
/* Virtual - unsigned offset is in the low 12 bits, assume the sign bit is set (i.e. the offset is always negative) */ | |||
jitVTableIndex = 0 - (instruction & 0x00000FFF); | |||
} | |||
#elif defined(J9VM_ARCH_AARCH64) | |||
/* To be filled */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these assert / #error for the time being for the same reasons as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to add assert here, too.
53eb5dd
to
65de4e1
Compare
Updated the files reflecting the review comments. I used
|
#4487 contains some other changes I had to make for building the aarch64 runtime. |
I think this can be merged if my changes after the review is OK. |
@@ -247,6 +247,13 @@ J9_EXTERN_BUILDER_SYMBOL(icallVMprJavaSendStatic1); | |||
#define JIT_LO_U64_RETURN_REGISTER gpr.numbered[0] | |||
#define JIT_HI_U64_RETURN_REGISTER gpr.numbered[1] | |||
|
|||
#elif defined(J9VM_ARCH_AARCH64) | |||
|
|||
#define JIT_STACK_OVERFLOW_SIZE_REGISTER gpr.numbered[18] // TODO: determine which register to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tie the //TODO to an issue #?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened Issue #4855 for this.
Jenkins test sanity xlinux jdk8 |
Fixed runtime/ddr/jitflagsddr.c. |
Build failed due to:
|
I have already fixed the problem with The Travis CI build is failing with the following error:
Pushing again without changing the code. |
This commit adds support for AArch64 in JIT-related files. Signed-off-by: knn-k <konno@jp.ibm.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Jenkins test sanity xlinux jdk8 |
This commit adds support for AArch64 in JIT-related files.
Signed-off-by: knn-k konno@jp.ibm.com