Replace calls to indexedTrampolineLookup with findHelperTrampoline #4746
Conversation
0xdaryl
added
comp:jit
depends:omr
0xdaryl
changed the title
|
Depends on eclipse-omr/omr#3570 |
|
Jenkins test sanity all jdk11 |
0xdaryl
changed the title
|
Un-WIPing as upstream changes have made it to openj9-omr. |
There was a problem hiding this comment.
This PR is the OpenJ9 piece of eliminating the FrontEnd function to lookup a helper trampoline, and instead ask the CodeCacheManager directly. For the most part, this is what the FrontEnd function does anyway.
One notable difference is that the OpenJ9 FrontEnd function indexedTrampolineLookup actually performs the lookup in a critical section. I'm not sure this is actually required as the helper trampolines never change in a code cache. Also, there are already direct calls to the CodeCacheManager::findHelperTrampoline throughout the compiler which would occur without the critical section. So, we're already wildly inconsistent on this. I chose to eliminate the critical section altogether. If anyone can think of the reason why it's needed then its easy for me to add back in as a specialization in the J9 version of CodeCacheManager.
The FrontEnd implementation of indexedTrampolineLookup will be deleted once uses are eliminated from upstream OMR.
| @@ -237,19 +238,16 @@ void J9::Recompilation::methodHasBeenRecompiled(void *oldStartPC, void *newStart | |||
|
|
|||
| uint32_t encodeRuntimeHelperBranchAndLink(TR_RuntimeHelper helper, uint8_t *cursor, TR_FrontEnd *fe) | |||
| { | |||
There was a problem hiding this comment.
This function needed a bit more love because of its inconsistent use of a single data type to represent the target address.
| @@ -64,103 +64,3 @@ uint32_t TR::ARMRecompilationSnippet::getLength(int32_t estimatedSnippetStart) | |||
| { | |||
| return 12; | |||
| } | |||
| /* | |||
There was a problem hiding this comment.
This commented out block of code has long been deprecated. It contained a reference to the indexedTrampolineLookup so I simply deleted the whole thing.
| @@ -723,3 +723,9 @@ J9::CodeCacheManager::printOccupancyStats() | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| intptrj_t | |||
There was a problem hiding this comment.
Provide a version of findHelperTrampoline that returns an intptrj_t type rather than void *. This eliminates a number of casts that would be immediately required when using this function.
There was a problem hiding this comment.
Why is there
OMR::CodeCacheManager::findHelperTrampoline(int32_t helperIndex, void *callSite)
OMR::CodeCacheManager::findHelperTrampoline(void *callingPC, int32_t helperIndex)
but only
J9::CodeCacheManager::findHelperTrampoline(int32_t helperIndex, void *callSite)
Wouldn't it be really easy for someone in OpenJ9 to accidentally switch the parameters and end up calling the OMR version?
EDIT: nvm, I see the comment about it being a temporary function; does this mean that eventually there will only be a single OMR function rather the two?
| @@ -213,14 +214,14 @@ TR::S390J9CallSnippet::generateInvokeExactJ2IThunk(TR::Node * callNode, int32_t | |||
| cursor += 2; | |||
|
|
|||
| TR::SymbolReference *helper = cg->symRefTab()->findOrCreateRuntimeHelper(TR_methodHandleJ2IGlue, false, false, false); | |||
| uintptrj_t destAddr = (uintptrj_t)helper->getMethodAddress(); | |||
| intptrj_t destAddr = (uintptrj_t)helper->getMethodAddress(); | |||
There was a problem hiding this comment.
nitpick, the cast is (uintptrj_t) but the type of destAddr is intptrj_t.
There was a problem hiding this comment.
Not a nit pick. Thanks for pointing that out. I'll fix and re-push.
I thought the critical section was using a monitor, but it's a VMAccessCriticalSection. I really can't think of why the JIT would need VM Access for the code cache. If I were grasping for straws, I'd say the only situation it might matter is under FSD (because of the jettisoning of the code cache)? |
My intention is to change the return type of this function. I introduced this version of the function temporarily in OpenJ9 to prevent build breakages because of the coordinated changes required in OMR. Next steps:
|
Ask the CodeCacheManager directly for the helper trampoline address. Change the return type of `CodeCacheManager::findHelperTrampoline` to an `intptrj_t` and fix up references. Introduce a J9 version of `findHelperTrampoline` to prevent breakages while upstream OMR is changed. Change some invocations of `findHelperTrampoline` to use the new form where the helper index is the first parameter. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
|
Jenkins test sanity all jdk11 |
Ask the CodeCacheManager directly for the helper trampoline address.
Change the return type of
CodeCacheManager::findHelperTrampolineto anintptrj_tand fix up references. Introduce a J9 version of
findHelperTrampolinetoprevent breakages while upstream OMR is changed.
Change some invocations of
findHelperTrampolineto use the new form where thehelper index is the first parameter.
Signed-off-by: Daryl Maier maier@ca.ibm.com