Skip to content
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.

Sign up for GitHub

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

Jump to bottom

AArch64: Add support for AArch64 in JIT-related files #4447

Merged
merged 1 commit into from Feb 28, 2019
Merged

AArch64: Add support for AArch64 in JIT-related files #4447

merged 1 commit into from Feb 28, 2019

Conversation

knn-k
Copy link
Contributor

@knn-k knn-k commented Jan 25, 2019

This commit adds support for AArch64 in JIT-related files.

Signed-off-by: knn-k konno@jp.ibm.com

@knn-k knn-k force-pushed the aarch64build2 branch 4 times, most recently from 1fda13d to 734e7bd Compare January 28, 2019 10:22
@knn-k knn-k force-pushed the aarch64build2 branch 2 times, most recently from 8e00cc4 to 49c0f3d Compare January 29, 2019 08:28
@knn-k
Copy link
Contributor Author

knn-k commented Jan 29, 2019

Corrected copyright year.

@knn-k
Copy link
Contributor Author

knn-k commented Jan 30, 2019

The change in runtime/compiler/module.xml suppresses building the JIT for the time being.

@knn-k knn-k changed the title WIP: AArch64: Add support for AArch64 in JIT-related files AArch64: Add support for AArch64 in JIT-related files Jan 30, 2019
@knn-k
Copy link
Contributor Author

knn-k commented Feb 1, 2019

Added changes in jilconsts.c.

This was referenced Feb 1, 2019
Merged
Merged
DanHeidinga
DanHeidinga reviewed Feb 6, 2019
View reviewed changes
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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the comment.

runtime/compiler/module.xml
@@ -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.*" />
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

runtime/compiler/runtime/MethodMetaData.c Outdated
@@ -1746,6 +1746,8 @@ void jitAddSpilledRegisters(J9StackWalkState * walkState, void * stackMap)
}
while (savedGPRs != 0);
}
#elif defined(TR_HOST_ARM64)
// To be filled
Copy link
Member

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

Copy link
Contributor Author

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 */
Copy link
Member

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?

Copy link
Contributor Author

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.

@knn-k knn-k force-pushed the aarch64build2 branch 2 times, most recently from 53eb5dd to 65de4e1 Compare February 7, 2019 03:30
@knn-k
Copy link
Contributor Author

knn-k commented Feb 7, 2019

Updated the files reflecting the review comments.

I used assert(0) for adding asserts:

  • MethodMetaData.c uses assert() in other functions in it.
  • JITInterface.hpp is included by both codert_vm/cnathelp.cpp and vm/BytecodeInterpreter.hpp. I don't know which Trace Assert to use in such a case.

@knn-k
Copy link
Contributor Author

knn-k commented Feb 7, 2019

#4487 contains some other changes I had to make for building the aarch64 runtime.

@knn-k knn-k mentioned this pull request Feb 12, 2019
Merged
@knn-k
Copy link
Contributor Author

knn-k commented Feb 18, 2019

I think this can be merged if my changes after the review is OK.

DanHeidinga
DanHeidinga reviewed Feb 22, 2019
View reviewed changes
runtime/codert_vm/cnathelp.cpp
@@ -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
Copy link
Member

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 #?

Copy link
Contributor Author

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.

This was referenced Feb 25, 2019
Closed
Closed
@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk8

@knn-k
Copy link
Contributor Author

knn-k commented Feb 25, 2019

Fixed runtime/ddr/jitflagsddr.c.

@DanHeidinga
Copy link
Member

Build failed due to:

10:28:00  In file included from jitflagsddr.c:24:0:
10:28:00  jitflagsddr.c:44:50: error: ‘host_AArch64’ undeclared here (not in a function); did you mean ‘host_AARCH64’?
10:28:00   J9DDRConstantTableEntryWithValue("host_AArch64", host_AArch64)

@DanHeidinga DanHeidinga self-assigned this Feb 26, 2019
@knn-k
Copy link
Contributor Author

knn-k commented Feb 26, 2019 •
edited

I have already fixed the problem with jitflagsddr.c in Jenkins build.

The Travis CI build is failing with the following error:

  • For target jdk_modules_java.base__the.java.base_batch:
    Connection attempt failed: Connection refused (Connection refused)
    Connection attempt failed: Connection refused (Connection refused)
    Connection attempt failed: Connection refused (Connection refused)
    Giving up
    IOException caught during compilation: Could not connect to server

Pushing again without changing the code.

@knn-k
AArch64: Add support for AArch64 in JIT-related files
9fce880 
This commit adds support for AArch64 in JIT-related files.

Signed-off-by: knn-k <konno@jp.ibm.com>
DanHeidinga
DanHeidinga approved these changes Feb 27, 2019
View reviewed changes
Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@DanHeidinga
Copy link
Member

Jenkins test sanity xlinux jdk8

@DanHeidinga DanHeidinga merged commit 0d213e5 into eclipse-openj9:master Feb 28, 2019
@knn-k knn-k deleted the aarch64build2 branch February 28, 2019 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanHeidinga DanHeidinga DanHeidinga approved these changes

Assignees

@DanHeidinga DanHeidinga

Labels
comp:vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@knn-k @DanHeidinga @pshipton