Improve x86 inline checkcast/instanceof sequences for array cast classes - #23383
Conversation
|
@a7ehuo : may I ask you to review this please? @vijaysun-omr FYI |
a7ehuo
left a comment
a7ehuo
left a comment
There was a problem hiding this comment.
Here is my review on the first commit. I'll review the second commit next Monday.
| logprintf(comp->getOption(TR_TraceCG), comp->log(), "Inline checkcast for [jlO : node=%p", node); | ||
| if (reportInlineObjectArrayCheck) | ||
| { | ||
| OMR::CStdIOStreamLogger::Stdout->printf("XXXXX Inline checkcast for [jlO : isCheckCast=%d (icall=%d) : %s\n", isCheckCast, (node->getOpCodeValue() == TR::icall), comp->signature()); |
There was a problem hiding this comment.
-
What is
XXXXXin the trace for? Should it be removed? -
Is
OMR::CStdIOStreamLoggerlogging into verbose vlog, or onto the console, or into compilation log? The reason I'm asking is that if this information should be logged into the compilation log as well at line 4164 ifcomp->getOption(TR_TraceCG)is true. -
The message
Inline checkcast for ...might be confusing since it could be instanceof orisAssignableFromcases.
There was a problem hiding this comment.
This was all used for debugging and discovery that I thought I would keep around in case it was useful. However, I'll just remove it.
| static char *disableInlineObjectArrayCheck = feGetEnv("TR_DisableInlineObjectArrayCheck"); | ||
|
|
||
| if (!disableInlineObjectArrayCheckCast && isCheckCast && clazz && TR::Compiler->cls.isClassArray(comp, clazz)) | ||
| bool isRelocatableCompile = comp->compileRelocatableCode() || comp->isOutOfProcessCompilation(); |
There was a problem hiding this comment.
isRelocatableCompile is not used in this commit. It should be removed in this commit. (I assumed likely it'd be used in the next commit)
| generateMemImmInstruction(TR::InstOpCode::TEST4MemImm4, node, | ||
| generateX86MemoryReference(romClassReg, offsetof(J9ROMClass, modifiers), cg), J9AccClassArray, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JE4, node, outlinedCallLabel, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JE4, node, outlinedHelperCallLabel, cg); |
There was a problem hiding this comment.
I wonder if this line could be updated as below to avoid going to the helper if it is not array for instanceof and isAssignableFrom cases since resultReg has already been initialized as zero.
generateLabelInstruction(TR::InstOpCode::JE4, node, isCheckCast ? outlinedHelperCallLabel : fallThruLabel, cg);| generateMemImmInstruction(TR::InstOpCode::TEST4MemImm4, node, | ||
| generateX86MemoryReference(romClassReg, offsetof(J9ROMClass, modifiers), cg), J9AccClassInternalPrimitiveType, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JNE4, node, outlinedCallLabel, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JNE4, node, outlinedHelperCallLabel, cg); |
There was a problem hiding this comment.
I wonder if this line could be updated as below to avoid going to the helper if it is primitive array for instanceof and isAssignableFrom cases since resultReg has already been initialized as zero.
generateLabelInstruction(TR::InstOpCode::JNE4, node, isCheckCast ? outlinedHelperCallLabel : fallThruLabel, cg);| if (!objectNode->isNonNull()) | ||
| { |
There was a problem hiding this comment.
Would it be a problem for isAssignableFrom(Class<?> cls)? If cls is null, NullPointerException (not return false) should be thrown, but the current behaviour would return 0/false by jumping to fallThruLabel.
// If the object is NULL, no exception is thrown for a checkcast and a 0
// is returned for an instanceof.
//
if (!objectNode->isNonNull())
{
generateRegRegInstruction(TR::InstOpCode::TESTRegReg(), node, objectReg, objectReg, cg);
generateLabelInstruction(TR::InstOpCode::JE4, node, fallThruLabel, cg);
}There was a problem hiding this comment.
When the isAssignableFrom is transformed, a NULLCHK is inserted for the receiver and the cast class. So a null value should have been filtered out before this code is reached.
|
Is it possible to show the sequence from a JIT trace log after instruction selection (to help with review) ? Thanks |
a7ehuo
left a comment
a7ehuo
left a comment
There was a problem hiding this comment.
Here is the second half of the review
| OMR::CStdIOStreamLogger::Stdout->printf("YYYYY Found inlineArrayExactCastClass : isCheckCast=%d : %s\n", isCheckCast, comp->signature()); | ||
| } | ||
|
|
||
| logprintf(comp->getOption(TR_TraceCG), comp->log(), "Inline instanceof/checkcast for const cast class array: node=%p", node); |
There was a problem hiding this comment.
Could the compilation log be updated to include isCheckCast value and mention isAssignableFrom case?
| // | ||
| if (reportInlineArrayExactCastClass) | ||
| { | ||
| OMR::CStdIOStreamLogger::Stdout->printf("YYYYY Found inlineArrayExactCastClass : isCheckCast=%d : %s\n", isCheckCast, comp->signature()); |
There was a problem hiding this comment.
What does YYYYY refer to? Should it be removed or updated to something more meaningful?
There was a problem hiding this comment.
Removed, as explained earlier.
| if (!objectNode->isNonNull()) | ||
| { | ||
| generateRegRegInstruction(TR::InstOpCode::TESTRegReg(), node, objectReg, objectReg, cg); | ||
|
|
||
| // checkcast leaves the operand stack unaffected | ||
| // instanceof returns 0 if the objectRef is null | ||
| // | ||
| TR::LabelSymbol *nullTargetLabel = isCheckCast ? fallThruLabel : notCastableDoNotCacheLabel; | ||
| generateLabelInstruction(TR::InstOpCode::JE4, node, nullTargetLabel, cg); | ||
| } |
There was a problem hiding this comment.
Does TR::icall corresponds directly to Java Class.isAssignableFrom? Will objectNode always be non null if the node is TR::icall? The reason I'm asking is that If cls is null, NullPointerException should be thrown.
There was a problem hiding this comment.
Yes. It seems the way we distinguish isAssignableFrom from the other cases is by the presence of an icall opcode. As mentioned in an earlier comment, there are NULLCHKs inserted that should catch the NPE cases.
| TR::Register *scratchReg2 = NULL; | ||
| TR::Register *scratchReg3 = NULL; | ||
|
|
||
| bool use64BitClasses = cg->comp()->target().is64Bit() && !TR::Compiler->om.generateCompressedObjectHeaders(); |
There was a problem hiding this comment.
The check cg->comp()->target().is64Bit() is redundant since this whole case 2 are wrapped under it
else if (!disableInlineArrayExactCastClass && !isRelocatableCompile && cg->comp()->target().is64Bit())| if (!scratchReg) | ||
| scratchReg = cg->allocateRegister(); | ||
|
|
||
| generateRegMemInstruction(TR::InstOpCode::LRegMem(), node, scratchReg, |
There was a problem hiding this comment.
Does it need to consider if use64BitClasses is true or false when generating TR::InstOpCode::LRegMem?
There was a problem hiding this comment.
I replaced all the parameterized instructions with 64-bit ones.
| if (IS_32BIT_SIGNED(componentClazzAddress)) | ||
| { | ||
| // TODO: Need a relocation for componentClazz | ||
| generateRegImmInstruction(TR::InstOpCode::CMPRegImm4(), node, objectClassLeafReg, (int32_t)componentClazzAddress, cg); | ||
| } | ||
| else | ||
| { | ||
| // TODO: Need a relocation for componentClazz | ||
| generateRegImm64Instruction(TR::InstOpCode::MOV8RegImm64, node, scratchReg, componentClazzAddress, cg); | ||
| generateRegRegInstruction(TR::InstOpCode::CMPRegReg(), node, objectClassLeafReg, scratchReg, cg); | ||
| } |
There was a problem hiding this comment.
Should it explicitly consider use64BitClasses being true and false cases here?
There was a problem hiding this comment.
I replaced all the parameterized instructions with 64-bit ones.
| intptr_t castClassArity = ((J9ArrayClass*)clazz)->arity; | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::CMPRegImm4(), node, scratchReg, (int32_t)castClassArity, cg); |
There was a problem hiding this comment.
Is it necessary to cast ((J9ArrayClass*)clazz)->arity from UDATA → intptr_t → int32_t? Should it be cast directly from UDATA → int32_t?
There was a problem hiding this comment.
I think I pulled that into a separate local variable because it was used for something else at some point, but now there is only one other use. I'll just fold the arity load into the genRegImmInstruction.
| #if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) | ||
| if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClass)) | ||
| { | ||
| static_assert(J9ClassArrayIsNullRestricted == 0x2000000, "Cannot do simple bit test for J9ClassArrayIsNullRestricted"); |
There was a problem hiding this comment.
The assert is currently written to require the value to be 0x2000000, but the assert message is unclear what is expected. Should it be updated to something like expect J9ClassArrayIsNullRestricted == 0x2000000 or J9ClassArrayIsNullRestricted must be 0x2000000 to be explicit?
| TR::Register *objectClassReg, | ||
| uintptr_t clazzAddress, | ||
| bool use64BitClasses, | ||
| TR::Register *scratchReg, |
There was a problem hiding this comment.
scratchReg is not used in this function.
| } | ||
| else | ||
| { | ||
| // TODO: process out of line, but could just throw a ClassCastException instead |
There was a problem hiding this comment.
This TODO: process out of line ... comment sounds a bit ambiguous to me since processing out of line is already happening. Do you mean for checkcast failure, instead of punting to the full helper, generate a dedicated OOL to throw ClassCastException? If so, could the TODO comment be updated to be more accurate?
There was a problem hiding this comment.
I just removed the TODO altogether and stated the current behaviour is for the out-of-line helper to throw the CastClassException.
0xdaryl
left a comment
0xdaryl
left a comment
There was a problem hiding this comment.
I have addressed all the comments (thank you!) in the next force push, in addition to the following:
- Create a special "case 2" for when the cast class is an array known at compile-time when the leaf class is final. The JIT logic is simplified and more readable, and I think the generated code is better.
- Rebased and reformatted the code per the new JIT code formatting guidelines
- Added more code documentation
| TR::Register *objectClassReg, | ||
| uintptr_t clazzAddress, | ||
| bool use64BitClasses, | ||
| TR::Register *scratchReg, |
| static char *disableInlineObjectArrayCheck = feGetEnv("TR_DisableInlineObjectArrayCheck"); | ||
|
|
||
| if (!disableInlineObjectArrayCheckCast && isCheckCast && clazz && TR::Compiler->cls.isClassArray(comp, clazz)) | ||
| bool isRelocatableCompile = comp->compileRelocatableCode() || comp->isOutOfProcessCompilation(); |
| logprintf(comp->getOption(TR_TraceCG), comp->log(), "Inline checkcast for [jlO : node=%p", node); | ||
| if (reportInlineObjectArrayCheck) | ||
| { | ||
| OMR::CStdIOStreamLogger::Stdout->printf("XXXXX Inline checkcast for [jlO : isCheckCast=%d (icall=%d) : %s\n", isCheckCast, (node->getOpCodeValue() == TR::icall), comp->signature()); |
There was a problem hiding this comment.
This was all used for debugging and discovery that I thought I would keep around in case it was useful. However, I'll just remove it.
| if (!objectNode->isNonNull()) | ||
| { |
There was a problem hiding this comment.
When the isAssignableFrom is transformed, a NULLCHK is inserted for the receiver and the cast class. So a null value should have been filtered out before this code is reached.
| // | ||
| if (reportInlineArrayExactCastClass) | ||
| { | ||
| OMR::CStdIOStreamLogger::Stdout->printf("YYYYY Found inlineArrayExactCastClass : isCheckCast=%d : %s\n", isCheckCast, comp->signature()); |
There was a problem hiding this comment.
Removed, as explained earlier.
| TR::LabelSymbol *notCastableUpdateCacheLabel = generateLabelSymbol(cg); | ||
|
|
||
| generateLabelInstruction(TR::InstOpCode::label, node, startLabel, cg); | ||
|
|
There was a problem hiding this comment.
Yes, comment added.
| #if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) | ||
| if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClass)) | ||
| { | ||
| static_assert(J9ClassArrayIsNullRestricted == 0x2000000, "Cannot do simple bit test for J9ClassArrayIsNullRestricted"); |
| } | ||
| else | ||
| { | ||
| // TODO: process out of line, but could just throw a ClassCastException instead |
There was a problem hiding this comment.
I just removed the TODO altogether and stated the current behaviour is for the out-of-line helper to throw the CastClassException.
| generateMemImmInstruction(TR::InstOpCode::TEST4MemImm4, node, | ||
| generateX86MemoryReference(romClassReg, offsetof(J9ROMClass, modifiers), cg), J9AccClassArray, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JE4, node, outlinedCallLabel, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JE4, node, outlinedHelperCallLabel, cg); |
| generateMemImmInstruction(TR::InstOpCode::TEST4MemImm4, node, | ||
| generateX86MemoryReference(romClassReg, offsetof(J9ROMClass, modifiers), cg), J9AccClassInternalPrimitiveType, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JNE4, node, outlinedCallLabel, cg); | ||
| generateLabelInstruction(TR::InstOpCode::JNE4, node, outlinedHelperCallLabel, cg); |
Case 1: objectRef instanceof Object[]Case 2: objectRef instanceof [I (final)Case 2: checkcast objectRef, [B (final)Case 3: objectRef instanceof [Lsun/reflect/generics/tree/FieldTypeSignature; (not final)Case 3: checkcast objectRef, [Ljava/lang/reflect/Member; (not final) |
|
I just realized that a further improvement here would be to avoid the inline sequence altogether (and handle out of line) if the leaf component class is an interface known at compile-time. I will save that enhancement for a subsequent PR to avoid destabilizing this one. |
|
jenkins test sanity all jdk21 |
|
Failures are all infrastructural (likely related to the ongoing Artifactory issues). This PR is entirely specific to x86 files, so there is no need to test on any platforms other than x86. |
|
Jenkins test sanity.functional,sanity.openjdk xlinux,win,osx jdk21 |
|
|
||
| generateLabelInstruction(TR::InstOpCode::JNE4, node, outlinedHelperCallLabel, cg); | ||
| } else { | ||
| generateRegInstruction(TR::InstOpCode::SETE1Reg, node, resultReg, cg); |
There was a problem hiding this comment.
Is there a further optimization possible that directly branches to the result of an if that is based on the instanceof ? I ask, because we had such ifinstanceof optimization in the past, and it may be that the SET operation here might have been replaced with a jump to the ifinstanceof target directly ?
There was a problem hiding this comment.
It is possible that you consider that a separate optimization to be done in a later PR, which is fine, but I was just asking about the plan.
There was a problem hiding this comment.
Yes, I think we talked about that a few weeks ago that the ifinstanceof optimizations on x86 seem to have been dropped over the years for some reason. You are right that there is an optimization opportunity here that can be exploited. See #23574.
| /** | ||
| * The only reason this is disabled for relocatable compiles (AOT and out-of-process | ||
| * compiles like JitServer) is because the support has not been implemented yet. | ||
| * Implementing the code relocations and appropriate frontend queries can be done, |
|
|
||
| // ----------------------------------------------------------------------- | ||
| // Perform trivial check whether objectClass is the same as the castClass. | ||
| // The castClass is known to be an array (implicitly final), so no |
There was a problem hiding this comment.
Small suggestion : I think your "implicitly final" phrase applies to the array class (in fact all array classes) and not to the component class. You may want to make that explicit by changing "implicitly final" to "all array classes are implicitly final"
| generateLabelInstruction(TR::InstOpCode::JNE4, node, castClassCacheMissLabel, cg); | ||
|
|
||
| // ---------------------------------------------------------------------- | ||
| // objectClass was found in the cache. Determine whether it was castable |
There was a problem hiding this comment.
Just so I am clear (and this comment makes it easier in the future), what does it mean to find a class in the cache, but it was not castable ?
There was a problem hiding this comment.
The low-bit on the class address stored in the cache indicates whether the cast succeeds or fails. So, for a class to be "cached but not castable" means it is stored in the cache but is low-tagged.
| * goto notCastableUpdateCache | ||
| * } | ||
| * | ||
| * if (objectClass->leaf == castClass->leaf) { |
There was a problem hiding this comment.
Is leaf the "leaf class" (going past all dimensions) or the "component class" (going past one dimension) ?
e.g. is "leaf" [X or X when one starts with [[X as the objectClass ?
There was a problem hiding this comment.
It is the leaf class, so X.
| * | ||
| * if (castClass->leaf is final) { | ||
| * if ((objectClass->leaf->depth <= castClass->leaf->depth) { | ||
| * if (castClass->leaf is an interface) { |
There was a problem hiding this comment.
Is punting to the helper what happens for interface checkcasts when encountered outside of the array case ?
There was a problem hiding this comment.
Not immediately. See [1] called from [2].
[1]
[2]
There was a problem hiding this comment.
should we have a follow on PR that uses similar logic here in the array case too ?
There was a problem hiding this comment.
@0xdaryl can I please get your answer on this ? I will merge it once this is done
There was a problem hiding this comment.
I clarified in the comments that this applies to all casts (checkcast, instanceof, Class.isAssignableFrom). Issue #23616 was created to track inlining interface array case, and that issue was referenced from the code in the latest force push.
mpirvu
left a comment
mpirvu
left a comment
There was a problem hiding this comment.
Remote compilations with JITServer should work in theory because we know the value of the j9class that the client wants to use. However, dereferencing the j9class pointer in the codegen should be avoided. For some of the dereferences we already have frontend queries we could use, while for others we ned to create such frontend queries.
| && cg->comp()->target().is64Bit()) { | ||
| TR_OpaqueClassBlock *castClassComponentClass = fej9->getComponentClassFromArrayClass(clazz); | ||
|
|
||
| J9Class *castClassLeafJ9Class = ((J9ArrayClass *)clazz)->leafComponentType; |
There was a problem hiding this comment.
Suggestion: use TR_J9VM::getLeafComponentClassFromArrayClass(TR_OpaqueClassBlock * arrayClass) instead of directly accessing a J9Class which would not work on JITServer.
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| // is not castable. Fall through to update the cache. | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| if (J9CLASS_IS_MIXED(castClassLeafJ9Class)) { |
There was a problem hiding this comment.
For JITServer we would need a frontend query for J9CLASS_IS_MIXED
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| generateX86MemoryReference(objectClassReg, offsetof(J9ArrayClass, arity), cg), cg); | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::CMP8RegImm4, node, scratchReg, | ||
| (int32_t)(((J9ArrayClass *)clazz)->arity), cg); |
There was a problem hiding this comment.
JITServer would need a frontend quert for clazz->arity
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
|
|
||
| #if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) | ||
| J9Class *castClassJ9Class = TR::Compiler->cls.convertClassOffsetToClassPtr(clazz); | ||
| if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClassJ9Class)) { |
There was a problem hiding this comment.
#define J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(clazz) J9_ARE_ALL_BITS_SET((clazz)->classFlags, J9ClassArrayIsNullRestricted)
JITServer will need a frontend query for this.
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| // Skip the subclass check if the castClassLeaf is final | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| bool castClassLeafIsInterface = J9ROMCLASS_IS_INTERFACE(castClassLeafJ9Class->romClass); |
There was a problem hiding this comment.
For JITServer benefit it's better to use J9::ClassEnv::romClassOf(TR_OpaqueClassBlock * clazz) for castClassLeafJ9Class->romClass.
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
a7ehuo
left a comment
a7ehuo
left a comment
There was a problem hiding this comment.
Thank you for the update! Here are my comments.
| if (clazz && TR::Compiler->cls.isClassArray(comp, clazz) && !isRelocatableCompile | ||
| && cg->comp()->target().is64Bit()) { |
There was a problem hiding this comment.
Just trying to understand, the original optimization for checkcast applies to isRelocatableCompile and 32-bit x86 as well. Now this new condition check narrows it to !isRelocatableCompile and target().is64Bit(). Is it intentional?
There was a problem hiding this comment.
It was intentional, but I've changed my reasoning and went back to the way it was originally. The first case is enabled for 32-bit and AOT/JitServer, and the two new cases apply only to 64-bit and non-AOT/JitServer (more implementation and testing work needs to be done).
| // If the object is NULL, no exception is thrown for a checkcast and a 0 | ||
| // is returned for an instanceof. |
There was a problem hiding this comment.
Could the comment be updated to explain why we don't need to worry about throwing exception for isAssignableFrom(null) case?
| uintptr_t clazzAddress = (uintptr_t)clazz; | ||
| J9Class *castClass = TR::Compiler->cls.convertClassOffsetToClassPtr(clazz); |
There was a problem hiding this comment.
castClass is not used in this commit
| generateRegMemInstruction(TR::InstOpCode::L8RegMem, node, scratchReg, | ||
| generateX86MemoryReference(objectClassReg, offsetof(J9Class, castClassCache), cg), cg); | ||
|
|
||
| if (use64BitClasses) { | ||
| if (IS_32BIT_SIGNED(clazzAddress)) { | ||
| generateRegImmInstruction(TR::InstOpCode::XOR8RegImm4, node, scratchReg, (int32_t)clazzAddress, cg); | ||
| } else { | ||
| if (!scratchReg2) | ||
| scratchReg2 = cg->allocateRegister(); | ||
| generateRegImm64Instruction(TR::InstOpCode::MOV8RegImm64, node, scratchReg2, clazzAddress, cg); | ||
| generateRegRegInstruction(TR::InstOpCode::XOR8RegReg, node, scratchReg, scratchReg2, cg); | ||
| } | ||
| } else { | ||
| generateRegImmInstruction(TR::InstOpCode::XOR4RegImm4, node, scratchReg, (int32_t)clazzAddress, cg); | ||
| } | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::TEST8RegImm4, node, scratchReg, ~1, cg); |
There was a problem hiding this comment.
Would this be a problem if use64BitClasses is false?
L8RegMem scratchReg, [objectClass->castClassCache] // scratchReg was loaded with L8RegMem, the upper 32 bits can contain unrelated data
XOR4RegImm4 scratchReg, clazz32 // xor 32-bit
TEST8RegImm4 scratchReg, ~1 // test 64-bit
There was a problem hiding this comment.
I don't think so. On 64-bit, the castClassCache field is a UDATA which is 64-bits, and when it is populated the full 64-bits are written. Since your last review, one of the paths in generateCastClassCacheUpdate was fixed to write 8 bytes, and now all paths through that code write a full 8 bytes. VM code always writes 8 bytes into this field.
For the XOR and TEST instructions, the operation is 64-bits and the 32-bit immediates are sign extended to 64-bits.
| generateRegMemInstruction(TR::InstOpCode::L8RegMem, node, objectClassLeafReg, | ||
| generateX86MemoryReference(objectClassReg, offsetof(J9ArrayClass, leafComponentType), cg), cg); | ||
|
|
There was a problem hiding this comment.
Does it need to consider use64BitClasses as false case for leafComponentType?
There is a bit inconsistency on when use64BitClasses should be considered: for example, when loading castClassCache in generateCastClassCacheUpdate, it does consider it. But in generateInlinedCheckCastOrInstanceOfForArrayClass, it doesn't. Then here we have J9ArrayClass.leafComponentType case.
There was a problem hiding this comment.
No. The use64BitClasses in generateCastClassCacheUpdate is used to understand the size of the cast class address and whether it will fit in a 32-bit immediate or not (and hence 32-bit immediate form instructions can be used). The castClassCache is a UDATA and the class should be stored as 8 bytes on 64-bit. Since your last review, one of the paths in generateCastClassCacheUpdate was fixed to write 8 bytes, and now all paths through that code write a full 8 bytes. VM code always writes 8 bytes into this field.
| * Handles class addresses of different sizes. | ||
| * | ||
| * @param[in] objectClassReg : register containing the destination object class | ||
| * @param[in[ clazzAddress : the class address to update in the cache |
| generateRegImmInstruction(TR::InstOpCode::XOR4RegImm4, node, scratchReg, (int32_t)clazzAddress, cg); | ||
| } | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::TEST8RegImm4, node, scratchReg, ~1, cg); |
There was a problem hiding this comment.
Could the magic numbers be defined as something like CAST_CLASS_CACHE_CASTABLE_BIT = 1 and CAST_CLASS_CACHE_MASK = ~1 for improved readability?
| } | ||
| } | ||
|
|
||
| static void generateInlinedCheckCastOrInstanceOfForArrayClass(TR::Node *node, TR_OpaqueClassBlock *clazz, |
There was a problem hiding this comment.
This method has gone rather long and complex with all three cases. Is it possible to refactor them into three separate functions for the 3 case: InlineObjectArrayCastClass, InlineFinalArrayCastClass, and InlineArrayExactCastClass?
| static char *disableInlineObjectArrayCheck = feGetEnv("TR_DisableInlineObjectArrayCheck"); | ||
| static char *disableInlineFinalArrayCastClass = feGetEnv("TR_DisableInlineFinalArrayClass"); | ||
| static char *disableInlineArrayExactCastClass = feGetEnv("TR_DisableInlineArrayExactCastClass"); |
There was a problem hiding this comment.
Should TR_DisableInlineObjectArrayCheck be updated as something like TR_DisableInlineObjectArrayCaseClass to be more consistent as case 2 & 3?
| // or not and exit appropriately. | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::TEST8RegImm4, node, scratchReg, 1, cg); |
There was a problem hiding this comment.
Similar question, should TEST here also consider use64BitClasses?
There was a problem hiding this comment.
No, by same rationale above.
|
I feel that this can be merged once the existing set of comments is addressed. So I am giving my approval for it to be actioned by Annabelle at her judgement (unless a non-trivial point comes out in response to the last set of comments). |
| * Handles class addresses of different sizes. | ||
| * | ||
| * @param[in] objectClassReg : register containing the destination object class | ||
| * @param[in[ clazzAddress : the class address to update in the cache |
| // Skip the subclass check if the castClassLeaf is final | ||
| // ---------------------------------------------------------------------- | ||
|
|
||
| bool castClassLeafIsInterface = J9ROMCLASS_IS_INTERFACE(castClassLeafJ9Class->romClass); |
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
|
|
||
| #if defined(J9VM_OPT_VALHALLA_FLATTENABLE_VALUE_TYPES) | ||
| J9Class *castClassJ9Class = TR::Compiler->cls.convertClassOffsetToClassPtr(clazz); | ||
| if (J9_IS_J9ARRAYCLASS_NULL_RESTRICTED(castClassJ9Class)) { |
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| generateX86MemoryReference(objectClassReg, offsetof(J9ArrayClass, arity), cg), cg); | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::CMP8RegImm4, node, scratchReg, | ||
| (int32_t)(((J9ArrayClass *)clazz)->arity), cg); |
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| generateLabelInstruction(TR::InstOpCode::JNE4, node, castClassCacheMissLabel, cg); | ||
|
|
||
| // ---------------------------------------------------------------------- | ||
| // objectClass was found in the cache. Determine whether it was castable |
There was a problem hiding this comment.
The low-bit on the class address stored in the cache indicates whether the cast succeeds or fails. So, for a class to be "cached but not castable" means it is stored in the cache but is low-tagged.
| && cg->comp()->target().is64Bit()) { | ||
| TR_OpaqueClassBlock *castClassComponentClass = fej9->getComponentClassFromArrayClass(clazz); | ||
|
|
||
| J9Class *castClassLeafJ9Class = ((J9ArrayClass *)clazz)->leafComponentType; |
There was a problem hiding this comment.
Thank you. I will defer this to a follow-on PR for AOT/JitServer support.
| generateRegMemInstruction(TR::InstOpCode::L8RegMem, node, objectClassLeafReg, | ||
| generateX86MemoryReference(objectClassReg, offsetof(J9ArrayClass, leafComponentType), cg), cg); | ||
|
|
There was a problem hiding this comment.
No. The use64BitClasses in generateCastClassCacheUpdate is used to understand the size of the cast class address and whether it will fit in a 32-bit immediate or not (and hence 32-bit immediate form instructions can be used). The castClassCache is a UDATA and the class should be stored as 8 bytes on 64-bit. Since your last review, one of the paths in generateCastClassCacheUpdate was fixed to write 8 bytes, and now all paths through that code write a full 8 bytes. VM code always writes 8 bytes into this field.
| generateRegImmInstruction(TR::InstOpCode::XOR4RegImm4, node, scratchReg, (int32_t)clazzAddress, cg); | ||
| } | ||
|
|
||
| generateRegImmInstruction(TR::InstOpCode::TEST8RegImm4, node, scratchReg, ~1, cg); |
| } | ||
| } | ||
|
|
||
| static void generateInlinedCheckCastOrInstanceOfForArrayClass(TR::Node *node, TR_OpaqueClassBlock *clazz, |
| if (clazz && TR::Compiler->cls.isClassArray(comp, clazz) && !isRelocatableCompile | ||
| && cg->comp()->target().is64Bit()) { |
There was a problem hiding this comment.
It was intentional, but I've changed my reasoning and went back to the way it was originally. The first case is enabled for 32-bit and AOT/JitServer, and the two new cases apply only to 64-bit and non-AOT/JitServer (more implementation and testing work needs to be done).
|
Jenkins test sanity xlinux,win,osx jdk17 |
|
Thanks for the changes, I approve |
|
Jenkins test sanity xlinux,win,osx jdk17 |
|
@a7ehuo : would you be able to sign off on these changes by end of week? |
a7ehuo
left a comment
a7ehuo
left a comment
There was a problem hiding this comment.
LGTM. Thank you very much for the update and clarification!
|
Thanks for the very thorough review @a7ehuo. It was very helpful. @vijaysun-omr : I think this can be merged at your convenience. |
…or [jlO Extend the current x86 checkcast optimization when the cast class is a java/lang/Object array to handle instanceof and Class.isAssignableFrom() cases. Also, move the inlining logic into a separate static function. Signed-off-by: Daryl Maier <maier@ca.ibm.com>
…al leaf class Signed-off-by: Daryl Maier <maier@ca.ibm.com>
Essentially inlines the checkcast/instanceof/isAssignableFrom() sequence when the cast class is an array that the VM implements [1], but specializes it for when the cast class is an array known at compile-time. It performs: * An exact equality check * A check for a match in the cast class cache * An arity check followed by a subclass check on the array leaf components when the arities are the same * Updates the cast class cache on success/failure in the same manner that the VM implementation does Any “unusual” cases are punted to the VM (e.g., mismatched arities, when the leaf class is an interface), as well as to throw the CastClassException if required. The opt can be disabled by setting `TR_DisableInlineArrayExactCastClass`. [1] https://github.com/eclipse-openj9/openj9/blob/9698be0c19fdc436c2fd6a0588dbafcfc0dd76fa/runtime/oti/VMHelpers.hpp#L589 Signed-off-by: Daryl Maier <maier@ca.ibm.com>
|
Force push only included changes to comments and a rebase. I don't believe re-running CI is necessary. |
|
Thanks for all these changes and the review responses. Merging. |
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
…upport to PR#23543 Replace J9Class* access with corresponding front end queries to enable JITServer support. A follow up to this PR eclipse-openj9#23383 Specifically these comments eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment) eclipse-openj9#23383 (comment)
Add support for SVM AOT to array check cast optimizations for x86. A follow up to this PR eclipse-openj9#23383 To address the issues raised in this issue eclipse-openj9#23510 Very much in the same vein as this PR eclipse-openj9#23662
4 participants
This PR makes two contributions to JIT generated code on x86-64:
[java/lang/ObjectExtend the current x86 checkcast optimization when the cast class is a
java/lang/Object array to handle instanceof and isAssignableFrom() cases.
Essentially inlines the checkcast/instanceof/isAssignableFrom() sequence when the
cast class is an array that the VM implements [1], but specializes it for when the
cast class is an array known at compile-time. It performs:
arities are the same
implementation does
Any “unusual” cases are punted to the VM (e.g., mismatched arities, when the leaf
class is an interface), as well as to throw the
CastClassExceptionif required.The opt can be disabled by setting
TR_DisableInlineArrayExactCastClass.[1]
openj9/runtime/oti/VMHelpers.hpp
Line 589 in 9698be0