Refactor SuperclassTest generators on Z

[matthewhall2]

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

java8 zos, zLinux
Jenkins personal

[matthewhall2]

@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]

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]

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]

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]

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]

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]

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.