Refactor SuperclassTest generators on Z #20906

matthewhall2 · 2025-01-10T14:30:44Z

Commons out code in genTestIsSuper and genInstanceOfOrCheckCastSuperClassTest into 3 helpers (genTestRuntimeFlags, genLoadAndCompareClassDepth, genCheckSuperclassArray)

Replaces calls to the old generators with calls to the 3 new

matthewhall2 · 2025-01-10T14:35:03Z

@r30shah please review

r30shah

Hi Matthew, I was under impression that genInstanceOfOrCheckcastSuperClassTest can be refactored/updated to something like genSuperClassTest which handles the cases for all three - instanceOf/ CHeckCast / isAssignableFrom to ensure that if there are any changes that happens in the future to class structure, we can make sure that only one place is changed and we do not need to go through all three places where super class test is generated to mae sure.
Please share the reason behind this (As you are using assignableFrom, may be some code that makes it difficult to use the single SuperClassTest function). so would like to know.

That being said, if we are going down the path of having SuperClassTest changed into three function, we should write a good comment explaining what it is doing, and also have properName to the helper function (May be testClassFlagsForSuperClassTest, testClassDepthForSuperClassTest).

r30shah · 2025-01-13T15:21:50Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+/**
+ * generates code to branch to <callHelperLabel> if any of the J9ROMClass modifier flags match the given flags <flags>
+ */
+static void genTestRuntimeFlags(TR::CodeGenerator *cg, TR::Node *node, TR::Register *classReg, int32_t toClassDepth, TR::LabelSymbol *callHelperLabel, TR_S390ScratchRegisterManager *srm, int32_t flags, const char *callerName)


Hmm, I wonder how are you using this function. The runtime check for interface or abstract class is used when the other tests in the checkcast / instance of fails and it is a safeguard to do so before loading the class from super class array. I would think of this test is always coming with super class test. That being said, can you share the logic behind this ?

Also genTestRuntimeFlags is not very suitable name for what it is doing (Rather misleading actually) as it is doing test to ensure that it is not interface / array before doing super class test.

r30shah · 2025-01-13T15:25:09Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+ * Loads the class depth of the toClass (if needed) as well as the depth of the fromClass.
+ * Genereates code to compare the 2 depths and branch to the failLabel if fromClassDepth <= toClassDepth
+ */
+static TR::Register *genLoadAndCompareClassDepth(TR::CodeGenerator *cg, TR::Node *node, TR::Register *toClassReg, int32_t toClassDepth, TR::Register *fromClassReg, TR::LabelSymbol *failLabel, TR_S390ScratchRegisterManager *srm, const char *callerName)


Same comment as above. Everything that it is doing is specific to Super Class test where some of the core-login behind class depth and array/interface should be the same. So would appreciate if you can share the reason behing this.

matthewhall2 · 2025-01-13T19:56:28Z

Hi Matthew, I was under impression that genInstanceOfOrCheckcastSuperClassTest can be refactored/updated to something like genSuperClassTest which handles the cases for all three - instanceOf/ CHeckCast / isAssignableFrom to ensure that if there are any changes that happens in the future to class structure, we can make sure that only one place is changed and we do not need to go through all three places where super class test is generated to mae sure. Please share the reason behind this (As you are using assignableFrom, may be some code that makes it difficult to use the single SuperClassTest function). so would like to know.

That being said, if we are going down the path of having SuperClassTest changed into three function, we should write a good comment explaining what it is doing, and also have properName to the helper function (May be testClassFlagsForSuperClassTest, testClassDepthForSuperClassTest).

@r30shah This change does handle all 3 cases. Did I miss a spot where genInstanceOfOrCheckcastSuperClassTest is used?

Regarding #20906 (comment),
The idea behind the separation is for the possibility of adding other inlined tests (interface, abstract, arrayclass) where we may want different behaviour for different cases of instanceOf vs. CheckCast vs. isAssignableFrom.

For example, if we still want the CHelper call for Array Classes for isAssignableFrom, but want to inline the test for interfaces, we can do something like

inlineCheckAssignableFromEvaluator(...)
{
...
genTestRuntimeFlags(..., callHelperLabel, ArrayClassFlag, ...) // branches to helper call on array class
genTestRuntimeFlags(..., handleInterfaceLabel, InterfaceFlag, ...) // branches to section that handles interface
...
}

But if we want to keep CheckCast the way it is right now, we just use

checkcastEvaluator(...)
{
...
genTestRuntimeFlags(..., callHelperLabel, ArrayClassFlag | InterfaceFlag, ...) // branches to helper call on array class or interface
...
}

Additionally, the runtime check for an Interface or Array Class is not just a safeguard. Only isAssignableFrom does a compile-time check of the castClass, instanceof and checkcast do not. For those 2 the check for Interface or Array Class is only done at runtime.
I forgot to change the instruction comment so we could pass in a specific comment based on what we are checking for, but I can fix that.

To sum it up the overall idea was to allow for greater expandability.

For #20906 (comment), similar to above, the idea behind separating it was so it could be used if we decide to inline all or part of the superclass test for Array Classes. This would allow us to do a similar class-depth-check shortcut. genLoadAndCompareClassDepth would need a slight modification to work for Array Classes, but that can be done if/when we make a decision about inlining the test.

r30shah · 2025-01-13T20:23:40Z

Thanks a lot @matthewhall2 for the explanation. Do we have a potential test that we would be doing when doing the if it is interface or not. Existing code for genInstanceOfOrCheckcastSuperClassTest is used with something like that in the context. For instance of and isInstanceOf, if the class is interface or abstract class, instanceof evaluator will check into on site circular buffer (Dynamic Cache) which will see if cast class and object class was compared by this site before and if yes what was the result. genInstanceOfOrCheckcastSuperClassTest was implemented with the intention of jumping to false label and helpercall/next test label in mind with fall through is always true result label. I do not expect things to be different when it comes to isAssignableFrom. To contrast the expandability aspect, I am thinking the changes/ places we may need to change if there are certain changes in the VM side that may require JIT to make changes in the way super class test is performed.

Only isAssignableFrom does a compile-time check of the castClass, instanceof and checkcast do not. For those 2 the check for Interface or Array Class is only done at runtime.

That is incorrect. Code that prepares the list of test to be generated at compile time for instance Of and checkCast [1] does that. It would be very inefficient if we do not check the information which is available at compile time and go all out with test to be performed at runtime. If we know that the cast class is not regular class at compile time, why do we even do super class test ? Only case where we would actually check class is interface or array class at runtime is when we have a dynamic cast class (Which will be result of when cast class is not compile time constant - which I think you can generate this test with Obj.getClass().isInstance(someObj))

[1].

openj9/runtime/compiler/codegen/J9TreeEvaluator.cpp

Line 1282 in b7d59c7

    
           uint32_t J9::TreeEvaluator::calculateInstanceOfOrCheckCastSequences(TR::Node *instanceOfOrCheckCastNode, InstanceOfOrCheckCastSequences *sequences, TR_OpaqueClassBlock **compileTimeGuessClass, TR::CodeGenerator *cg, InstanceOfOrCheckCastProfiledClasses *profiledClassList, uint32_t *numberOfProfiledClass, uint32_t maxProfiledClass, float * topClassProbability, bool *topClassWasCastClass)

matthewhall2 · 2025-01-14T16:49:51Z

Thank you for the clarification @r30shah .
For the interface case, yes I was planning on adding the existing DynamicCacheObjectClassTest and CastClassCacheTest and CastClassCacheTest. For new tests, I wanted to look into doing the ITable walk inline, potentially with vector instructions based on the size of table.

For abstract class, I want to add the skip over the class equality test and add the existing CompileTimeGuessClassTest and looking into adding the CastClassCacheTest (that one is currently not done for instanceof or checkcast).

I was also planning on looking into the same tests we already have but for Array Classes. For example, the inline superclass test it not generated for any of instanceof, checkcast, isassignableFrom for Array Classes.

r30shah · 2025-01-14T17:09:49Z

Are these tests you are talking about, you are planning to add for Class.isAssignableFrom ? or checkCast / instanceOf (The iTable walk routine was already something I think we should implement and thing about reducing the inline code we are generated) But that IMO is beyond the scope of improving the Class.isAssignableFrom from the current implementation.

If this approach is allowing you to make progress, I am OK with you separating the Super Class test generation to different helper function if that makes sense to you with the current implementation state. But I would factor in the places where we may need to change - Currently, it is only the genSuperClassTest that is needed to be changed and also you more relatable names for the function.

matthewhall2 · 2025-01-27T14:30:16Z

@r30shah can you review again please? Tests for java8 are passing on a development branch. Tests for this branch are currently running (no difference between this branch and the dev branch, just did not want to push a bunch of changes while I figured out the issue).

Considering how often Class.isAssignableFrom, as well as checkcast and instanceof, are called on interfaces, I think it would be best to keep the genTestModifierFlags helper method separate, but I have combined the genTestClassDepth and genTestSuperclassArray helpers into a single method.
I will post an update with the breakdown of each case (interface, abstract, array, normal) in the issue related to this PR.

I'll fix the line endings when the jenkins tests finish to avoid having to restart the tests.

r30shah

I have done half review (Atleast the code used in check-cast) some of the comments would be same, so please apply general changes to both evaluators, Will do complete review when you address the comments.

r30shah · 2025-01-27T22:22:14Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-               * through helper call if result of SuperClassTest turned out to be false.
-               */
+
+            int32_t flags = J9AccInterface | J9AccClassArray;


Couple of things, as J9AccInterface / J9AccInterface is constant, we should use constant here. Also why do we need to pass the flag to the genTestModifierFlags - Is it because that will be different for checkCast/InstantOf/CheckAssignableFrom ?

Yes, the checks are different for isAssignableFrom (we also check for Abstract there).

Also, since we are likely to be adding inlined tests for the case where castClass is an interface, the flags passed in will change in the future (genTestModifierFlags will be called multiple times. I have changed the design of genTestModifierFlags to accommodate this more efficiently. Now it skips the load when we already have the flags)

r30shah · 2025-01-27T22:26:02Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-               */
+
+            int32_t flags = J9AccInterface | J9AccClassArray;
+            genTestModifierFlags(cg, node, castClassReg, castClassDepth, callLabel, srm, flags, "checkcast");


What is the purpose of passing the checkcast (callerName) constant string here ? If there is an assert that is being thrown in the function - that assert should be followed by the stack trace, so we would know where that is coming from, apart from that if you are using this in trace message, in the trace log you would know which node you are generating instruction for.

r30shah · 2025-01-27T22:27:53Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4573,6 +4540,8 @@ J9::Z::TreeEvaluator::checkcastEvaluator(TR::Node * node, TR::CodeGenerator * cg
            if (comp->getOption(TR_TraceCG))
               traceMsg(comp, "%s: Emitting Profiled Class Test\n", node->getOpCode().getName());
            TR::Register *arbitraryClassReg1 = srm->findOrCreateScratchRegister();
+            if (comp->getOption(TR_TraceCG))


I think this was the temp. change for the issue you are debugging - May not need this - we can see what instruction we generated to know how that instruction was used.

r30shah · 2025-01-27T22:28:05Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4592,6 +4561,8 @@ J9::Z::TreeEvaluator::checkcastEvaluator(TR::Node * node, TR::CodeGenerator * cg
               traceMsg(comp, "%s: Emitting Compile Time Guess Class Test\n", node->getOpCode().getName());
            cg->generateDebugCounter(TR::DebugCounter::debugCounterName(comp, "checkCastStats/(%s)/CompTimeGuess", comp->signature()),1,TR::DebugCounter::Undetermined);
            TR::Register *arbitraryClassReg2 = srm->findOrCreateScratchRegister();
+            if (comp->getOption(TR_TraceCG))
+               traceMsg(comp, "Scratch reg for compile time guess test: %s\n", arbitraryClassReg2->getRegisterName(comp));


Same comment as before.

r30shah · 2025-01-27T22:28:46Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4619,6 +4590,8 @@ J9::Z::TreeEvaluator::checkcastEvaluator(TR::Node * node, TR::CodeGenerator * cg
            if (comp->getOption(TR_TraceCG))
               traceMsg(comp,"%s: Emitting CastClassCacheTest\n",node->getOpCode().getName());
            TR::Register *castClassCacheReg = srm->findOrCreateScratchRegister();
+            if (comp->getOption(TR_TraceCG))
+               traceMsg(comp, "Scratch reg for cast class cache test: %s\n", castClassCacheReg->getRegisterName(comp));


Same comment as before - If you want to increase readability of the code generated code - may be add instruction comment

r30shah · 2025-01-27T23:21:24Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4492,12 +4445,22 @@ J9::Z::TreeEvaluator::checkcastEvaluator(TR::Node * node, TR::CodeGenerator * cg
            if (comp->getOption(TR_TraceCG))
               traceMsg(comp, "%s: Class Not Evaluated. Evaluating it\n", node->getOpCode().getName());
            castClassReg = cg->evaluate(castClassNode);
+            if (comp->getOption(TR_TraceCG))
+               traceMsg(comp, "cast class is in reg: %s\n", castClassReg->getRegisterName(comp));


Same comment as in other places.

r30shah · 2025-01-27T23:21:38Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

            break;
         case LoadObjectClass:
            if (comp->getOption(TR_TraceCG))
               traceMsg(comp, "%s: Loading Object Class\n",node->getOpCode().getName());
            objClassReg = cg->allocateRegister();
+            if (comp->getOption(TR_TraceCG))
+               traceMsg(comp, "obj class is in reg: %s\n", objClassReg->getRegisterName(comp));


Same comment as in other places.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah · 2025-01-27T23:31:11Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-            int32_t castClassDepth = castClassNode->getSymbolReference()->classDepth(comp);
+            if (castClassDepth == -1)
+               {
+               cg->generateDebugCounter("matthew/checkcast/depth_unknown", 1, TR::DebugCounter::Undetermined);


Why do you generate a dynamic debug counter for info that is known at compile time. Also - please refrain naming your debug counter like you have here - keep it more standard. Think about asking some customer to get information using debug counter and asking them to use the string you have used here ?

yes, sorry, all the counters with "matthew/" were for debugging or to count number of invocations on interface vs abstract vs ... . I have removed them

r30shah · 2025-01-27T23:31:19Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+               }
+            else
+               {
+               cg->generateDebugCounter("matthew/checkcast/depth_known", 1, TR::DebugCounter::Undetermined);


Same comment as above

matthewhall2 · 2025-02-05T14:34:57Z

please review again @r30shah . Adding ICF labels to instanceof and checkcast is working with no issues.
Instead of starting the ICF in the LoadObjClass case, I've modified calculateInstanceOfOrCheckCastSequences to include a StartICF sequence when required.

r30shah · 2025-02-11T22:05:43Z

runtime/compiler/codegen/J9TreeEvaluator.cpp

+               {
+               memmove(&sequences[j + 2], &sequences[j], (i - j) * sizeof(InstanceOfOrCheckCastSequences));
+               sequences[j] = LoadObjectClass;
+               sequences[j + 1] = StartICF;


Although, I am OK with this approach as well - Now as we know the reason behind you getting those Assert message about assigning register within ICF, can we actually start the ICF region in the beginning on the evaluator (Probably after object is evaluated) conservatively ? Would that cause any issue ?

Also, if we are going this way (If conservatively putting ICF works then I would prefer that option) this function is being used by P and ARM code gen as well and we need to make sure their evaluator does not run into this functional issue or hit any assert with this test added. Either changes need to be made there or guard this by Z changes.

No, that should not cause any issue. I'll make the change

No, that should not cause any issue. I'll make the change

Making change in other codegen or here ?

Taking out this change and adding the ICF label conservatively in the Z evaluators

r30shah · 2025-02-11T22:13:13Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-   // convert this offset to a real J9Class pointer
-#endif
-   if (dynamicCastClass)
+   TR_ASSERT(flags < UINT_MAX && flags > 0,


Now as you are passing a flag, the assert comment and the check does not match. I think this assert should make more sense in the evaluator calling this.

r30shah · 2025-02-11T22:16:15Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+   TR::Register *scratchReg = modiferReg == NULL ? srm->findOrCreateScratchRegister() : modiferReg;
+   TR::Instruction *cursor = NULL;
+   TR_Debug * debugObj = cg->getDebug();
+   if (!modiferReg)


Can you explain the purpose behind this register being passed ? I would assume this is something you are planning to pass from Class.isAssignableFrom - Please use the name that explains the purpose.

The register is generated by this function, and returned when keepScratchReg is true.
The purpose is so that is can be passed back in when the function is called again to avoid having to load the flags from memory again

r30shah · 2025-02-11T22:17:48Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-            // For dynamic cast class genInstanceOfOrCheckcastSuperClassTest will generate branch to either helper call or dynamicCacheTest depending on the next generated test.
-            dynamicCastClass = genInstanceOfOrCheckcastSuperClassTest(node, cg, objClassReg, castClassReg, castClassDepth, falseLabel, *(iter+1) == DynamicCacheDynamicCastClassTest ? dynamicCacheTestLabel : callLabel, srm);
+
+            dynamicCastClass = castClassDepth == -1;


Suggested change

dynamicCastClass = castClassDepth == -1;

dynamicCastClass = ( castClassDepth == -1 );

r30shah · 2025-02-11T22:18:28Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -11643,17 +11552,15 @@ static bool inlineIsAssignableFrom(TR::Node *node, TR::CodeGenerator *cg)
   cg->decReferenceCount(node->getFirstChild());
   cg->decReferenceCount(node->getSecondChild());

+


Unnecessary extra line.

r30shah · 2025-02-11T22:24:55Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+            TR::LabelSymbol *callHelperLabel = *(iter+1) == DynamicCacheDynamicCastClassTest ? dynamicCacheTestLabel : callLabel;
+            genTestModifierFlags(cg, node, castClassReg, castClassDepth, callHelperLabel, srm, flags);
+            genSuperclassTest(cg, node, castClassReg, castClassDepth, objClassReg, falseLabel, srm);
+            dynamicCastClass = castClassDepth == -1;


Already computed. Remove this line.

r30shah · 2025-02-11T22:40:11Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-               * through helper call if result of SuperClassTest turned out to be false.
-               */
+
+            const int32_t flags = J9AccInterface | J9AccClassArray;


Suggested change

const int32_t flags = J9AccInterface | J9AccClassArray;

const int32_t flags = ( J9AccInterface | J9AccClassArray ) ;

I think you missed this one.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah · 2025-02-11T22:41:55Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-      else if (outOfBound)
+      fromClassDepthReg = srm->findOrCreateScratchRegister();
+      cursor = generateRXInstruction(cg, loadOp, node, fromClassDepthReg,
+         generateS390MemoryReference(fromClassReg, offsetof(J9Class, classDepthAndFlags) + bytesOffset, cg));


insert tabs to move this line indented after cg.

r30shah · 2025-02-11T22:42:12Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+      {
+      toClassDepthReg = srm->findOrCreateScratchRegister();
+      cursor = generateRXInstruction(cg, loadOp, node, toClassDepthReg,
+            generateS390MemoryReference(toClassReg, offsetof(J9Class, classDepthAndFlags) + bytesOffset, cg));


Same comment as below.

Same comment as peviously with indentation.

r30shah

Apart from the slight design change of your implementation of genTestModifierFlags, some minor nitpicks. Overall changes looks ok to me. Once you make the change, please launch the sanity tests internally and share the link of the builds with me on slack. I will also on parallel launch tests here.

r30shah · 2025-02-13T14:36:10Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-   bool eliminateSuperClassArraySizeCheck = (!dynamicCastClass && (castClassDepth < comp->getOptions()->_minimumSuperclassArraySize));
-
+   // don't want to waste a load and/or AND if we can check the flags at compile time
+   if (classDepth != -1) return NULL;


Is there a point of calling this function when cast class is not known at compile time. I think caller should ensure that this function is not called for known compile time known class and we should have Fatal assert here,

r30shah · 2025-02-13T14:54:57Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-   )
+/** Used in conjunction with genSuperclassTest as part of the Superclass test for checkcast, instanceof, Class.isAssignableFrom
+ * Generates branch instruction to jump to <handleFlagsLabel> when at least one of the modifiers of the class in <classReg> matches the given flags
+ * If modifier reg is non-null, no load instruction will be generated, and modiferReg if assumed to contain the J9ROMClass modifiers.


I would have preferred the caller to allocate the register (either through SRM or itself) and pass it here if it wants to use the loaded modifiers off the classReg and skip it here, as that way caller have control on the registers being used in the evaluator and if the caller does not pass the register, the function would get a scratch register and reclaim it at the end, fits in with the design of the SRM - and you do not end up in scenario where someone mistakenly call this function with j9classModifierFlagsReg and keepScratchReg set to false - which later is reclaimed in this function - which may not something someone want.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah · 2025-02-13T14:59:48Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-
-      cursor = generateRILInstruction(cg, TR::InstOpCode::NILF, node, scratch1Reg, static_cast<int32_t>((J9AccInterface | J9AccClassArray)), cursor);
+      cursor = generateRXInstruction(cg, TR::InstOpCode::L, node, scratchReg,
+            generateS390MemoryReference(scratchReg, offsetof(J9ROMClass, modifiers), cg));


Same suggestion as above.

r30shah · 2025-02-13T15:01:01Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp


+   if (!keepScratchReg)


As I mentioned, I do find it error prune for someone to incorrectly use this function. It should be simple, if the caller wants to keep the register, it should allocate one and load the value itself, rather than having this back and forth.

r30shah · 2025-02-13T15:01:36Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+      {
+      toClassDepthReg = srm->findOrCreateScratchRegister();
+      cursor = generateRXInstruction(cg, loadOp, node, toClassDepthReg,
+            generateS390MemoryReference(toClassReg, offsetof(J9Class, classDepthAndFlags) + bytesOffset, cg));


Same comment as peviously with indentation.

r30shah · 2025-02-13T15:15:23Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+      cursor = generateRXInstruction(cg, loadOp, node, toClassDepthReg,
+            generateS390MemoryReference(toClassReg, offsetof(J9Class, classDepthAndFlags) + bytesOffset, cg));
+      if (debugObj)
+         debugObj->addInstructionComment(cursor, "Load castClass depth");


I am ok with using toClass and fromClass from castClass and objectClass (Whch is the more common and understandable terminology with instanceof and checkcast and would have preferred that) respectively. In the comment keep this consistent.

r30shah · 2025-02-13T15:22:54Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

@@ -4397,7 +4241,7 @@ J9::Z::TreeEvaluator::checkcastEvaluator(TR::Node * node, TR::CodeGenerator * cg
   TR::Register                  *castClassCopyReg = NULL;
   TR::Register                  *resultReg = NULL;

-   // We need here at maximum two scratch registers so forcing scratchRegisterManager to create pool of two registers only.
+   int32_t castClassDepth = castClassNode->getSymbolReference()->classDepth(comp);


I think use of this variable is only needed when we are generating SuperClassTest, please move it closer to the point it is being used. This will reduce the scope of live register in the compiled C code.

r30shah · 2025-02-13T15:27:05Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

+            TR_ASSERT(flags < UINT_MAX && flags > 0, "superclass test::(J9AccInterface | J9AccClassArray) is not a 32-bit number\n");
+
+            // For dynamic cast class to next 2 calls will generate branch to either helper call or dynamicCacheTest depending on the next generated test.
+            TR::LabelSymbol *callHelperLabel = *(iter+1) == DynamicCacheDynamicCastClassTest ? dynamicCacheTestLabel : callLabel;


Suggested change

TR::LabelSymbol *callHelperLabel = *(iter+1) == DynamicCacheDynamicCastClassTest ? dynamicCacheTestLabel : callLabel;

TR::LabelSymbol *callHelperLabel = (*(iter+1) == DynamicCacheDynamicCastClassTest) ? dynamicCacheTestLabel : callLabel;

Commons out code in genTestIsSuper and genInstanceOfOrCheckCastSuperClassTest into 2 helpers (genTestModifierFlags, genSuperclassTest) Replaces calls to the old generators with calls to the 2 new methods. Adds ICF labels to checkcast and instanceof evaluators. Adds StartICF sequence to InstanceOfOrCheckCastSequences. Signed-off-by: Matthew Hall <[email protected]>

r30shah

Approving this, you may have missed one recommendation. Gonna let is pass by.

r30shah · 2025-02-14T14:27:32Z

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

-               * through helper call if result of SuperClassTest turned out to be false.
-               */
+
+            const int32_t flags = J9AccInterface | J9AccClassArray;


I think you missed this one.

runtime/compiler/z/codegen/J9TreeEvaluator.cpp

r30shah · 2025-02-14T14:33:30Z

jenkins test sanity all jdk11,jdk21

matthewhall2 · 2025-02-18T14:09:50Z

failures are all on x86 or ppc64_aix and are not related to this change @r30shah

r30shah · 2025-02-24T14:09:30Z

I agree, changes in this PR should not have any impact on other platform. I am relaunching sanity run with JDK21 before merging this today.

r30shah · 2025-02-24T14:10:04Z

jenkins test sanity zlinux jdk21

r30shah · 2025-02-24T22:14:29Z

S390 failure in https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal/331 was related to #18048.
Based on the previous tests and recent test, I am merging this change.

matthewhall2 force-pushed the refactor_superclass_tests branch 2 times, most recently from f845b5b to b5b2cd2 Compare January 10, 2025 14:34

r30shah reviewed Jan 13, 2025

View reviewed changes

matthewhall2 force-pushed the refactor_superclass_tests branch from b5b2cd2 to a74f843 Compare January 27, 2025 14:13

r30shah requested changes Jan 27, 2025

View reviewed changes

matthewhall2 force-pushed the refactor_superclass_tests branch 7 times, most recently from b582627 to 1847f0e Compare January 28, 2025 15:45

matthewhall2 force-pushed the refactor_superclass_tests branch 9 times, most recently from f04dc58 to d64e97a Compare February 5, 2025 14:10

r30shah requested changes Feb 11, 2025

View reviewed changes

matthewhall2 force-pushed the refactor_superclass_tests branch from d64e97a to f89af39 Compare February 12, 2025 22:18

matthewhall2 force-pushed the refactor_superclass_tests branch 13 times, most recently from 3c89953 to 6ecc0a0 Compare February 13, 2025 15:23

matthewhall2 requested a review from r30shah February 13, 2025 15:28

r30shah requested changes Feb 13, 2025

View reviewed changes

matthewhall2 force-pushed the refactor_superclass_tests branch 3 times, most recently from 30a46e1 to d0fb5ab Compare February 13, 2025 16:36

matthewhall2 force-pushed the refactor_superclass_tests branch from d0fb5ab to 42b06f3 Compare February 13, 2025 16:37

matthewhall2 requested a review from r30shah February 13, 2025 18:46

r30shah approved these changes Feb 14, 2025

View reviewed changes

r30shah closed this Feb 24, 2025

r30shah reopened this Feb 24, 2025

r30shah merged commit 31df02c into eclipse-openj9:master Feb 24, 2025
46 of 52 checks passed

	dynamicCastClass = castClassDepth == -1;
	dynamicCastClass = ( castClassDepth == -1 );

		@@ -11643,17 +11552,15 @@ static bool inlineIsAssignableFrom(TR::Node node, TR::CodeGenerator cg)
		cg->decReferenceCount(node->getFirstChild());
		cg->decReferenceCount(node->getSecondChild());

	const int32_t flags = J9AccInterface \| J9AccClassArray;
	const int32_t flags = ( J9AccInterface \| J9AccClassArray ) ;

	TR::LabelSymbol callHelperLabel = (iter+1) == DynamicCacheDynamicCastClassTest ? dynamicCacheTestLabel : callLabel;
	TR::LabelSymbol callHelperLabel = ((iter+1) == DynamicCacheDynamicCastClassTest) ? dynamicCacheTestLabel : callLabel;

Refactor SuperclassTest generators on Z #20906

Refactor SuperclassTest generators on Z #20906

Conversation

matthewhall2 commented Jan 10, 2025 • edited Loading

matthewhall2 commented Jan 10, 2025

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

matthewhall2 commented Jan 13, 2025 • edited Loading

r30shah commented Jan 13, 2025 • edited Loading

matthewhall2 commented Jan 14, 2025 • edited Loading

r30shah commented Jan 14, 2025

matthewhall2 commented Jan 27, 2025 • edited Loading

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

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 commented Feb 5, 2025

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

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

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

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

r30shah left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r30shah commented Feb 14, 2025

matthewhall2 commented Feb 18, 2025

r30shah commented Feb 24, 2025

r30shah commented Feb 24, 2025

r30shah commented Feb 24, 2025

matthewhall2 commented Jan 10, 2025 •

edited

Loading

matthewhall2 commented Jan 13, 2025 •

edited

Loading

r30shah commented Jan 13, 2025 •

edited

Loading

matthewhall2 commented Jan 14, 2025 •

edited

Loading

matthewhall2 commented Jan 27, 2025 •

edited

Loading

r30shah left a comment •

edited

Loading