Change Class.isAssignableFrom to use CHelper instead of JNI call on Z #20590

matthewhall2 · 2024-11-13T18:26:17Z

Calls to Class.isAssignableFrom will now use CHelper instead of a JNI call.

note: as discussed (@r30shah ), this change is not enabled yet on Z. It will be enabled when the inlined tests are added to checkAssignabeEvaluator.
vmfarm sanity check for this branch

The following tests were run on a branch with the change enabled:
vmfarm
personal s390x
personal s390x (unstable re-run)

matthewhall2 · 2024-11-13T18:34:38Z

extended.openjdk failure looks like its #12807

r30shah · 2024-11-13T18:38:59Z

runtime/compiler/z/codegen/J9CodeGenerator.cpp

+bool J9::Z::CodeGenerator::supportsInliningOfIsAssignableFrom()
+   {
+   return false; // will be taken out once new inlined tests are added
+   static const bool disableInliningOfIsAssignableFrom = feGetEnv("TR_DisableInliningOfIsAssignableFrom") != NULL;


Why do you need this ? Parent Codegenerator class implementation will return false. As you are not yet enabling this, We should take this override out of this PR.

r30shah · 2024-11-13T18:39:52Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -11627,6 +11626,60 @@ static bool inlineIsAssignableFrom(TR::Node *node, TR::CodeGenerator *cg)
   return true;
   }

+static TR::Register* checkAssignableEvaluator(TR::Node *node, TR::CodeGenerator *cg)


I would rename this to inlineCheckAssignableFromEvaluator.

r30shah · 2024-11-13T18:44:16Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+   return resultReg;
+   }
+
+TR::Register *J9::Z::TreeEvaluator::directCallEvaluator(TR::Node *node, TR::CodeGenerator *cg)


Why do you need this ? directCallEvaluator will call the code-generator's inline direct call function which is where you should check and call checkAssignableFromEvaluator.

[1]. https://github.com/eclipse-omr/omr/blob/3da49aa3aaed9217fb02d5ab3d069aa7142f89ba/compiler/z/codegen/OMRTreeEvaluator.cpp#L11171

r30shah · 2024-11-13T18:44:51Z

runtime/compiler/z/codegen/S390CHelperLinkage.cpp

+static bool getIsFastPathOnly(TR::Node * callNode, TR::CodeGenerator * cg) {
+   auto opCode = callNode->getOpCodeValue();
+
+   if (opCode == TR::instanceof) {


Please match the braces with rest of the code style

r30shah · 2024-11-21T14:58:36Z

runtime/compiler/z/codegen/S390CHelperLinkage.cpp

+      return true;
+      }
+
+   TR::MethodSymbol * methodSymbol = callNode->getSymbol()->getMethodSymbol();


What is the use if methodSymbol?

r30shah · 2024-11-21T15:47:07Z

runtime/compiler/z/codegen/S390CHelperLinkage.cpp

@@ -250,6 +250,19 @@ class RealRegisterManager
   TR::CodeGenerator*   _cg;
   };

+static bool getIsFastPathOnly(TR::Node * callNode, TR::CodeGenerator * cg)


Consumer of this function should be the S390CHelperLinkage only. Can you make this member and private?

r30shah · 2024-11-21T15:48:00Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -11479,13 +11479,12 @@ J9::Z::TreeEvaluator::VMarrayCheckEvaluator(TR::Node *node, TR::CodeGenerator *c
 /////////////////////////////////////////////////////////////////////////////////
 static bool inlineIsAssignableFrom(TR::Node *node, TR::CodeGenerator *cg)
   {
-   static char *disable = feGetEnv("TR_disableInlineIsAssignableFrom");
+   static char *disable = feGetEnv("TR_DisableInliningOfIsAssignableFrom");


This change is not relevant unless you are using TR_DisableInliningOfIsAssignableFrom somehow.
Can you remove it ?

r30shah · 2024-11-21T15:50:07Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+   TR::LabelSymbol *helperCallLabel = generateLabelSymbol(cg);
+   TR::LabelSymbol *doneLabel = generateLabelSymbol(cg);
+
+   int8_t numOfPostDepConditions = (thisClassReg == checkClassReg)? 2 : 3;


Did you encounter case where thisClassReg and checkClassReg are same ? I would prefer to call addPostConditionIfNotAlreadyInserted in that case.

I would like to move your comment from closer to OOL call to here as a TODO highlighting that there are incremental changes coming in to add more inlined tests. Or comment on why you chose to make call in the Out of line.

You haven't addressed the first part of the comment. Please respond or fix this.

r30shah · 2024-11-21T15:52:38Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+     }
+   deps->addPostCondition(resultReg, TR::RealRegister::AssignAny);
+
+   generateRIInstruction(cg, TR::InstOpCode::LHI, node, resultReg, 1); // load initial value for result


Why do we need to load initial value in resultReg ? You need to only allocate it and pass it to dispatch call.

that's for the inlined tests being added later. Those tests won't set the value, but jump to the "done" section on true (or fallthrough to the next test on fail). It's not the only way to do it (I could load 1 in the done/pass section), but it's what checkCast does so I went with that

I would like this changes to be as performant as possible and we do not need this setting of result value - in this code.

If you take a look at other evaluators, we do have tendency to write long evaluators which I think we should avoid and break them into a smaller functions which are easy to maintain. For this PR, I think it is OK, but as we start adding incremental changes to improve this, keep this in mind - InstanceOf and CheckCast evaluators does call out separate functions to generate specific tests.

r30shah · 2024-11-21T15:55:54Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+   generateS390LabelInstruction(cg, TR::InstOpCode::label, node, helperCallLabel);
+   resultReg = TR::TreeEvaluator::performCall(node, false, cg);
+
+   generateS390BranchInstruction(cg, TR::InstOpCode::BRC, TR::InstOpCode::COND_BRC, node, doneLabel); // skip over fail label


What is failLabel?

similar to above, when inlining is doable and the inlined tests fail, we want to set the value in resultReg to 0 under a failLabel. I'll take it out for now but it will be added back for the inlined tests

r30shah

One nitpick - Overall this incremental changes looks good to me.

r30shah · 2024-11-22T20:23:57Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+   TR::LabelSymbol *helperCallLabel = generateLabelSymbol(cg);
+   TR::LabelSymbol *doneLabel = generateLabelSymbol(cg);
+
+   int8_t numOfPostDepConditions = (thisClassReg == checkClassReg)? 2 : 3;


You haven't addressed the first part of the comment. Please respond or fix this.

r30shah · 2024-11-22T20:26:15Z

@matthewhall2 Please squash the commits to single, I do not think it logically makes sense to separate into two commits here.

r30shah

Last nitpick, LGTM

r30shah · 2024-11-25T15:38:18Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4326,6 +4326,7 @@ genInstanceOfOrCheckCastNullTest(TR::Node* node, TR::CodeGenerator* cg, TR::Regi
      }
   }

+


Remove extra new line

- took out inlined tests - call CHelper in OOL code section - note: while only calling the CHelper does improve perfance over inlined + JNI call, more inlined tests will added in a later PR to further improve performance Signed-off-by: Matthew Hall <[email protected]> - enables transformation of call to Class.isAssignableFrom to Signed-off-by: Matthew Hall <[email protected]>

r30shah · 2024-11-25T15:52:52Z

Jenkins test sanity zlinux jdk21

r30shah · 2024-11-25T20:27:43Z

Merging this one as test has passed.

matthewhall2 force-pushed the assignableFrom_useCHelper branch 2 times, most recently from 4c9f74e to 87ce953 Compare November 13, 2024 18:30

r30shah requested changes Nov 13, 2024

View reviewed changes

matthewhall2 force-pushed the assignableFrom_useCHelper branch 9 times, most recently from edd3d3e to 2742494 Compare November 13, 2024 21:43

matthewhall2 requested a review from r30shah November 13, 2024 21:45

r30shah requested changes Nov 21, 2024

View reviewed changes

matthewhall2 force-pushed the assignableFrom_useCHelper branch 6 times, most recently from 7127cc4 to 91ec6f6 Compare November 21, 2024 18:08

matthewhall2 requested a review from r30shah November 21, 2024 21:04

r30shah approved these changes Nov 22, 2024

View reviewed changes

matthewhall2 force-pushed the assignableFrom_useCHelper branch from 91ec6f6 to e25fddf Compare November 25, 2024 14:34

matthewhall2 requested a review from r30shah November 25, 2024 14:36

matthewhall2 force-pushed the assignableFrom_useCHelper branch from e25fddf to 71377fa Compare November 25, 2024 15:34

r30shah approved these changes Nov 25, 2024

View reviewed changes

matthewhall2 force-pushed the assignableFrom_useCHelper branch from 71377fa to 024edf7 Compare November 25, 2024 15:41

r30shah merged commit fc778d6 into eclipse-openj9:master Nov 25, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Class.isAssignableFrom to use CHelper instead of JNI call on Z #20590

Change Class.isAssignableFrom to use CHelper instead of JNI call on Z #20590

matthewhall2 commented Nov 13, 2024 •

edited

Loading

matthewhall2 commented Nov 13, 2024

r30shah Nov 13, 2024

r30shah Nov 13, 2024

r30shah Nov 13, 2024

r30shah Nov 13, 2024

r30shah Nov 21, 2024

r30shah Nov 21, 2024

r30shah Nov 21, 2024

r30shah Nov 21, 2024

r30shah Nov 22, 2024

r30shah Nov 21, 2024

matthewhall2 Nov 21, 2024 •

edited

Loading

r30shah Nov 21, 2024

r30shah Nov 21, 2024

matthewhall2 Nov 21, 2024

r30shah left a comment

r30shah Nov 22, 2024

r30shah commented Nov 22, 2024

r30shah left a comment

r30shah Nov 25, 2024

matthewhall2 Nov 25, 2024

r30shah commented Nov 25, 2024

r30shah commented Nov 25, 2024

		@@ -4326,6 +4326,7 @@ genInstanceOfOrCheckCastNullTest(TR::Node* node, TR::CodeGenerator* cg, TR::Regi
		}
		}

Change Class.isAssignableFrom to use CHelper instead of JNI call on Z #20590

Change Class.isAssignableFrom to use CHelper instead of JNI call on Z #20590

Conversation

matthewhall2 commented Nov 13, 2024 • edited Loading

matthewhall2 commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewhall2 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah commented Nov 22, 2024

r30shah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah commented Nov 25, 2024

r30shah commented Nov 25, 2024

matthewhall2 commented Nov 13, 2024 •

edited

Loading

matthewhall2 Nov 21, 2024 •

edited

Loading