AArch64: Implement PicBuilder.spp #5246
Conversation
knn-k
force-pushed
the
aarch64runtime4
branch
2 times, most recently
from
c1e215a
to
d69e237
Compare
|
This depends on eclipse-omr/omr#3707 |
|
Is this a WIP only because it has a dependency on another PR, or are you still making changes to it? If it's ready for me to look at (even as a WIP) please let me know. |
|
I am working to fill the unimplemented functions such as |
knn-k
changed the title
|
I am separating unimplemented functions to a new PR. |
|
Kicking the build again with no update in the commit. |
|
|
||
| // Encoding of CPIndex field in USC snippet (helperOffset: 8 bits, JVMPIEnabled: 1 bit, cpIndex: 23 bits) | ||
|
|
||
| .set J9TR_USCSnippet_JVMPIEnabled, 0x00800000 |
There was a problem hiding this comment.
JVMPI was deprecated in Java 6. Why is JVMPI still relevant here?
There was a problem hiding this comment.
It was taken from the arm version. Removing the related code.
| @@ -90,35 +242,101 @@ _virtualUnresolvedHelper: | |||
| _interfaceCallHelper: | |||
| hlt #0 // Not implemented yet | |||
|
|
|||
| // | |||
There was a problem hiding this comment.
A comment describing the high-level purpose of this code would be helpful.
| // 00: 5 helpers | ||
| // 40: 5 sync helpers | ||
|
|
||
| .set J9TR_SyncJ2ITableOffset, 40 |
There was a problem hiding this comment.
Why don't you move this (and its comment) down to where the table is actually defined? It feels out of place here.
There was a problem hiding this comment.
Moved both the constant definition and the table.
| // 00: 5 helpers | ||
| // 40: 5 sync helpers | ||
|
|
||
| .set J9TR_SyncJ2ITableOffset, 40 |
There was a problem hiding this comment.
This symbol refers to an offset in the "J2ITable", however the table is not actually called that. This is a bit confusing. Why not call the "staticGlueTable" the "staticJ2IGlueTable" (or some variant) instead?
There was a problem hiding this comment.
Changed it as J9TR_staticGlueTableSyncOffset.
| .text | ||
| .align 2 | ||
|
|
||
| // in: x0 = address of branch instruction |
There was a problem hiding this comment.
Please add a comment describing what this function does.
| .set J9TR_SCSnippet_returnAddress, 0 | ||
| .set J9TR_SCSnippet_method, 8 | ||
| .set J9TR_USCSnippet_CP, 16 | ||
| .set J9TR_USCSnippet_CPIndex, 24 |
There was a problem hiding this comment.
This doesn't match what the UnresolvedCallSnippet lays out. Please reconcile:
0 : code cache RA
8 : methodAddress (or 0 for unresolved)
16 : lock word
20 : cp address
28 : cp index
There was a problem hiding this comment.
I had overlooked the lock word. Corrected the offsets.
|
|
||
| // Static/special call snippet (and additional fields for unresolved snippet) | ||
|
|
||
| .set J9TR_SCSnippet_returnAddress, 0 |
There was a problem hiding this comment.
Can you call this codeCacheReturnAddress to help identify it as the RA in the code cache? It's ambiguous whether this is the RA in the snippet or the RA in the CC.
There was a problem hiding this comment.
Changed it as J9TR_SCSnippet_codeCacheReturnAddress.
| lsr x1, x28, #J9TR_USCSnippet_HelperOffsetShift // get helper offset | ||
| mov x0, x2 // recover method | ||
| bl jitMethodIsSync // is method synchronized? | ||
| lsr x2, x28, #(J9TR_USCSnippet_HelperOffsetShift+1) // save helper offset for refreshHelper |
There was a problem hiding this comment.
This should be +2 to get a division-by-four effect on the offset in the snippet (since the offsets are multiples of 8).
There was a problem hiding this comment.
Changed the shift amount.
| ldr x3, [x27, #J9TR_SCSnippet_method] | ||
| tbnz x3, #1, L_USSGclinitCase // branch if the LSB (the "clinit" bit) was set in the resolved address | ||
| ldr x28, [x27, #J9TR_USCSnippet_CP] // get CP | ||
| ldr x8, [x27, #J9TR_USCSnippet_CPIndex] // get CP index & flags |
There was a problem hiding this comment.
The cpIndex is already in x28 from above and I don't believe any path has clobbered it yet. Why not use that instead of loading? You'll have to find a different register for the cpAddr since you just loaded it into x28.
| tbnz x3, #1, L_USSGclinitCase // branch if the LSB (the "clinit" bit) was set in the resolved address | ||
| ldr x28, [x27, #J9TR_USCSnippet_CP] // get CP | ||
| ldr x8, [x27, #J9TR_USCSnippet_CPIndex] // get CP index & flags | ||
| and x8, x8, #(~J9TR_USCSnippet_HelperOffset) // remove helper offset from CP index |
There was a problem hiding this comment.
Previously you masked out the JVMPI bit on the offset as well. Notwithstanding my comment above about JVMPI support being deprecated, if for some reason it's needed then should you mask it off here too?
|
Updated a file reflecting the review comments, but not complete yet. |
|
I separated the flushICache change to PR #5908. |
This commit implements some functions in PicBuilder.spp for aarch64, such as _interpreterUnresolvedStaticGlue and _interpreterVoidStaticGlue. Signed-off-by: knn-k <konno@jp.ibm.com>
|
This is an AArch64-specific change. A Jenkins build is pointless. |
This commit adds a symbol missing in PicBuilder.spp for aarch64, L_rewriteBranch. eclipse-openj9#5246 added a reference to it but I forgot to define the label. Signed-off-by: knn-k <konno@jp.ibm.com>
This commit implements some functions in PicBuilder.spp for aarch64,
such as _interpreterUnresolvedStaticGlue and _interpreterVoidStaticGlue.
Signed-off-by: knn-k konno@jp.ibm.com