-
Notifications
You must be signed in to change notification settings - Fork 764
Generate inline superclass test for Class.isAssignableFrom on Z #20855
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
Generate inline superclass test for Class.isAssignableFrom on Z #20855
Conversation
093a9de
to
e0ce9ec
Compare
32f0761
to
4da8d70
Compare
4da8d70
to
6d94a59
Compare
d4d8676
to
ce999ca
Compare
@r30shah can you review please? |
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRC, node, failLabel); | ||
} | ||
} | ||
genTestIsSuper(cg, node, fromClassReg, toClassReg, sr1, sr2, NULL, NULL, toClassDepth, failLabel, successLabel, helperCallLabel, deps, NULL, false, NULL, NULL); |
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.
getTestIsSuper
is used by checkAssignableFrom old code and old instance of and checkcast code. We should make the changes is new isAssignableFrom with intention to clean-up old code in mind. I would take a look at genTestIsSuper
and genInstanceOfOrCheckcastSuperClassTest
and refactor common code to the single function. Currently the code in both is repeated twice with some subtle changes
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.
refactored in #20906
e49baeb
to
63470fb
Compare
63470fb
to
b5725a5
Compare
7e72a6a
to
a883a0c
Compare
adf87a1
to
1b98b52
Compare
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.
Thanks @matthewhall2 , Some minor nitpicks, but overall I feel changes are good. Once you address the comments, please launch internal build for Java 8 for both z/OS and Linux on Z with the changes enabled for sanity.
} else { | ||
classSymRef = classNode->getSymbolReference(); | ||
} |
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.
This does not match the compiler coding style.
} | ||
|
||
// try to find class depth | ||
if (!isClassNodeLoadAddr && ((classNode->getOpCodeValue() != TR::aloadi) || (classNode->getSymbolReference() != comp->getSymRefTab()->findJavaLangClassFromClassSymbolRef()) || |
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.
Nitpick and goes to the personal coding style so take it as suggestion,
In multiline if condition, I would add the body in curly braces, just for making it clear.
TR::SymbolReference *symRef = classRef->getOpCode().hasSymbolReference() ? classRef->getSymbolReference() : NULL; | ||
TR::StaticSymbol *classSym = ((NULL != symRef) && !symRef->isUnresolved()) ? (symRef ? symRef->getSymbol()->getStaticSymbol() : NULL) : NULL; | ||
TR_OpaqueClassBlock * clazz = (NULL != classSym) ? (TR_OpaqueClassBlock *) classSym->getStaticAddress() : NULL; | ||
classDepth = (NULL != clazz) ? (int32_t)TR::Compiler->cls.classDepthOf(clazz) : -1; |
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.
classDepth = (NULL != clazz) ? (int32_t)TR::Compiler->cls.classDepthOf(clazz) : -1; | |
classDepth = (NULL != clazz) ? static_cast<int32_t>(TR::Compiler->cls.classDepthOf(clazz)) : -1; |
TR::Register *fromClassReg = cg->evaluate(node->getFirstChild()); | ||
TR::Register *toClassReg = cg->evaluate(node->getSecondChild()); | ||
TR::Compilation *comp = cg->comp(); | ||
if (comp->getOption(TR_TraceCG)) |
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.
This code will be executed in the instruction selection, does this traceMsg serve any purpose when looking into the logs?
TR::Instruction *cursor = NULL; | ||
|
||
if (comp->getOption(TR_TraceCG)) | ||
traceMsg(comp,"%s: Emitting Class Equality Test\n",node->getOpCode().getName()); |
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.
Extra indentation.
cg->generateDebugCounter(TR::DebugCounter::debugCounterName(comp, "isAssignableFromStats/(%s)/ClassEqualityTest", comp->signature()),1,TR::DebugCounter::Undetermined); | ||
cursor = generateS390CompareAndBranchInstruction(cg, TR::InstOpCode::getCmpRegOpCode(), node, toClassReg, fromClassReg, TR::InstOpCode::COND_BE, successLabel, false, false); | ||
if (debugObj) | ||
debugObj->addInstructionComment(cursor, "class equality test"); |
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.
Extra indentation
debugObj->addInstructionComment(cursor, "toclass depth > fromClass depth at compile time - fast fail"); | ||
} | ||
srm->stopUsingRegisters(); | ||
goto fastFail; |
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.
Although we have some places in VM and GC code where we have used goto
statement, we certainly have not used it in the compiler code, given the poor readability and maintainability, Can we avoid using that?
1310ca9
to
940890b
Compare
2dc9282
to
6491f80
Compare
thanks @r30shah , I've addressed the comments |
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.
@matthewhall2 - Can you share the instruction selection log with the recent changes ? Also some nitpicks.
* if equal, we are done. If not, fall through to helper call | ||
*/ | ||
generateS390CompareAndBranchInstruction(cg, TR::InstOpCode::getCmpRegOpCode(), node, toClassReg, fromClassReg, TR::InstOpCode::COND_BE, successLabel, false, false); | ||
TR_S390ScratchRegisterManager *srm = cg->generateScratchRegisterManager(2);; |
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.
Double semicolon at the end.
d8813a9
to
f0b0ca1
Compare
Here's the selection log
which is from the call tests from the new changes: http://vmfarm.rtp.raleigh.ibm.com/build_info.php?build_id=98329 |
f0b0ca1
to
74405a7
Compare
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
Previously when performing Class.isAssignableFrom, only the trivial case of class equality was done inline and any other case relied on a callout to a C Helper. With this change, an inlined superclass test is added along with a castClassCache test, avoiding the C Helper in the most common cases. Additionally, this change checks for class depth at compile time when possible to fast-fail when fromClassDepth <= toClassDepth. Signed-off-by: Matthew Hall <[email protected]>
74405a7
to
2a39218
Compare
jenkins test sanity zlinux jdk8,jdk17,jdk21 |
jdk21 sanity.openjdk fail looks like its #18463 |
jenkins test sanity.openjdk zlinux jdk21 |
All test passes. Merging the changes. |
Before this change, only the class equality test was done inline for Class.isAssignableFrom, and other cases relied on a C Helper.
Now, the common case of the SuperClass test is done inline, along with a
castclasscache
check. This allows us to avoid calling the C Helper in the case of normal (non-interface) classes.Additionally we now also check, when possible, the class depths at compile time so we can fast-fail when fromClassDepth <= toClassDepth
This PR does not enable the change. It will be enabled in #20860