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

Generate inline class equality test for Class.isAssignableFrom on Z #20801

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Generate inline class equality test for Class.isAssignableFrom on Z #20801

merged 1 commit into from
Dec 11, 2024

Conversation

matthewhall2
Copy link
Contributor

@matthewhall2 matthewhall2 commented Dec 10, 2024 •
edited
Loading

Adds class equality check as first check; calls C Helper in OOL section on fail.

This change is not yet enabled, but will be enabled once remaining inlined tests are added.

vm-farm this branch (mz,xz), sanity aft comments
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/25470/

The following are tests run on a branch with this change enabled:
vm-farm test branch (mz, xz), sanity aft comments
https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/25464/

@matthewhall2 matthewhall2 force-pushed the assignableFrom_genClassEqualityTest branch 6 times, most recently from 389b284 to 0e85cb3 Compare December 10, 2024 18:06
@matthewhall2 matthewhall2 marked this pull request as ready for review December 10, 2024 19:44
@matthewhall2
Copy link
Contributor Author

@r30shah please review

@matthewhall2 matthewhall2 changed the title Generate inline class equality test Generate inline class equality test for Class.isAssignableFrom on Z Dec 10, 2024
@matthewhall2 matthewhall2 force-pushed the assignableFrom_genClassEqualityTest branch from 0e85cb3 to 2969a8a Compare December 10, 2024 19:45
r30shah
r30shah requested changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Overall looks good, some minor comments

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated
@@ -4486,6 +4486,12 @@ genInstanceOfOrCheckCastNullTest(TR::Node* node, TR::CodeGenerator* cg, TR::Regi
}
}

static void genInlineClassEqualityTest(TR::Node * node, TR::CodeGenerator * cg, TR::Compilation * comp, TR::Register * toClassReg, TR::Register * fromClassReg, TR::LabelSymbol * successLabel) {
cg->generateDebugCounter(TR::DebugCounter::debugCounterName(comp, "checkCastStats/(%s)/Equal", comp->signature()),1,TR::DebugCounter::Undetermined);
Copy link
Contributor

Choose a reason for hiding this comment

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

This debug counter name is not suitable for Class.isAssignableFrom. For the equality test, I feel we do not need separate function as it is only generating one instruction.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated
deps->addPostConditionIfNotAlreadyInserted(toClassReg, TR::RealRegister::AssignAny);
deps->addPostCondition(resultReg, TR::RealRegister::AssignAny);

generateS390LabelInstruction(cg, TR::InstOpCode::label, node, successLabel, deps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Your internal control flow ends at doneLabel, successLabel does not need register dependency condition.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated
deps->addPostCondition(thisClassReg, TR::RealRegister::AssignAny);
deps->addPostConditionIfNotAlreadyInserted(checkClassReg, TR::RealRegister::AssignAny);
deps->addPostCondition(resultReg, TR::RealRegister::AssignAny);
genInlineClassEqualityTest(node, cg, cg->comp(), toClassReg, fromClassReg, successLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

First instruction from which internal control flow starts is the instruction generated for class equality - make sure that you generate a label instruction before that and mark it as start internal control flow,.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

generateS390LabelInstruction(cg, TR::InstOpCode::label, node, successLabel, deps);
generateRIInstruction(cg, TR::InstOpCode::getLoadHalfWordImmOpCode(), node, resultReg, 1);

generateS390LabelInstruction(cg, TR::InstOpCode::label, node, doneLabel, deps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Your internal control flow ends here. Mark this to end ICF

@matthewhall2 matthewhall2 force-pushed the assignableFrom_genClassEqualityTest branch 2 times, most recently from 2969a8a to 81d885e Compare December 10, 2024 20:52
@matthewhall2 matthewhall2 requested a review from r30shah December 10, 2024 20:52
r30shah
r30shah requested changes Dec 10, 2024
View reviewed changes
runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated
@@ -11818,9 +11817,17 @@ TR::Register *J9::Z::TreeEvaluator::inlineCheckAssignableFromEvaluator(TR::Node
generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRC, node, doneLabel); // exit OOL section
outlinedSlowPath->swapInstructionListsWithCompilation();

generateS390LabelInstruction(cg, TR::InstOpCode::label, node, successLabel, deps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Register dependency should be attached to the end of internal control flow label. We do not need it here.

@matthewhall2
Generate inline class equality test for Class.isAssignableFrom on Z
cba0a22 
adds class equality check as first check; calls C Helper in OOL section
on fail

Signed-off-by: Matthew Hall <[email protected]>
@matthewhall2 matthewhall2 force-pushed the assignableFrom_genClassEqualityTest branch from 81d885e to cba0a22 Compare December 10, 2024 21:11
@matthewhall2 matthewhall2 requested a review from r30shah December 10, 2024 21:19
r30shah
r30shah approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

Looks good to me, Please share the snippet from Instruction selection log

@r30shah
Copy link
Contributor

r30shah commented Dec 10, 2024

jenkins test sanity zlinux jdk11,jdk21

@r30shah
Copy link
Contributor

r30shah commented Dec 11, 2024

I am merging this change based on the testing done by @matthewhall2 .

@r30shah r30shah merged commit 11968f8 into eclipse-openj9:master Dec 11, 2024
9 checks passed
@matthewhall2
Copy link
Contributor Author

Looks good to me, Please share the snippet from Instruction selection log

here you go

[     0x3ff6c242ac0]                          LG      &GPR_0086, Shadow[<javaLangClassFromClass>] 48(GPR_0082)
[     0x3ff6c242b90]                          LG      GPR_0084, Shadow[<classFromJavaLangClass>] 8(&GPR_0086)
[     0x3ff6c242f20]                          LG      &GPR_0090, Shadow[<javaLangClassFromClass>] 48(GPR_0080)
[     0x3ff6c242ff0]                          LG      GPR_0088, Shadow[<classFromJavaLangClass>] 8(&GPR_0090)
[     0x3ff6c2432b0]                          Label L0047:     # (Start of internal control flow)
[     0x3ff6c243390]                          CGRJ    GPR_0088,GPR_0084,Label L0046,BH(mask=0x8),
[     0x3ff6c243480]                          BRC     NOP(0xf), Outlined Label L0044
[     0x3ff6c246fa0]                          Label L0046:
[     0x3ff6c247080]                          LGHI    GPR_0132,0x1
[     0x3ff6c2477d0]                          assocreg
[     0x3ff6c247210]                          Label L0045:     # (End of internal control flow)
POST:
{AssignAny:GPR_0084:R} {AssignAny:GPR_0088:R} {AssignAny:GPR_0132:R}

------------------------------

[     0x3ff6c2435b0]                          Outlined Label L0044:
[     0x3ff6c245920]                          LGR     GPR_0131,GPR13
[     0x3ff6c245ea0]                          STG     GPR5,#411 32(GPR13)
[     0x3ff6c246560]                          assocreg
 PRE:
{GPR2:GPR_0131:R} {GPR3:GPR_0084:R} {GPR4:GPR_0088:R}
[     0x3ff6c245f70]                          BRASL   GPR_0098,0x0000000000000000
POST:
[     0x3ff6c246ca0]                          assocreg
[     0x3ff6c2466f0]                          LG      GPR5,#412 32(GPR13)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r30shah r30shah r30shah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewhall2 @r30shah