Use front-end implementation of the instanceOfOrCheckCast routines #6909
Conversation
There was a problem hiding this comment.
extern "C" {
IDATA
instanceOfOrCheckCast(J9Class *instanceClass, J9Class *castClass)
{
return VM_VMHelpers::inlineCheckCast(instanceClass, castClass, true) ? TRUE : FALSE;
}
IDATA
instanceOfOrCheckCastNoCacheUpdate(J9Class *instanceClass, J9Class *castClass)
{
return VM_VMHelpers::inlineCheckCast(instanceClass, castClass, false) ? TRUE : FALSE;
}
} /* extern "C" */
The implementation of instanceOfOrCheckCastNoCacheUpdate is different than instanceOfOrCheckCast and I don't know whether we can substitute one for another. maybe we need another frontend query?
I wonder how come we haven't hit this issue on x86
|
I think the cacheUpdate option is there for profiling purposes. Just a guess but perhaps we didn't see it because it's not functionally incorrect. With regards to the change in the common J9TreeEvaluator.cpp, the routines where I made this change are only invoked by the Power and Z codgens. So that's why that particular issue wasn't seen on x86. |
|
With regards to the new query, I can add a similar noCacheUpdate version of this routine. The current Server implementation of this routine ( In the new routine, we can use the same implementation as |
Let's do that please. |
[skip ci] Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
dchopra001
force-pushed
the
instanceCheckcast
branch
from
03fbae1
to
124189a
Compare
|
@mpirvu Done. This is good for another look. |
|
@mpirvu This is ready for another review. |
runtime/compiler/env/VMJ9Server.cpp
Outdated
| { | ||
| if (instanceClass == castClass) | ||
| if (instanceClass == castClass) |
There was a problem hiding this comment.
Fixed here: 3148b1f
runtime/compiler/env/VMJ9Server.cpp
Outdated
| if (cacheUpdate) | ||
| stream->write(JITServer::MessageType::VM_instanceOfOrCheckCast, instanceClass, castClass); | ||
| else | ||
| stream->write(JITServer::MessageType::VM_instanceOfOrCheckCastNoCacheUpdate, instanceClass, castClass); |
There was a problem hiding this comment.
The original implementation has a flaw in that we keep the romMapMonitor while sending a message which can be a lenghty operation. Could you please fix by ending the critical sectin before sending any messages?
There was a problem hiding this comment.
This should be addressed here: 3148b1f. If we end up at this point in this if block, we no longer do any remote calls and instead will fall through to the final remote call at the end of this routine
runtime/compiler/env/VMJ9Server.hpp
Outdated
| @@ -167,6 +167,8 @@ class TR_J9ServerVM: public TR_J9VM | |||
| virtual uintptrj_t getFieldOffset(TR::Compilation * comp, TR::SymbolReference* classRef, TR::SymbolReference* fieldRef) override { return 0; } // safe answer | |||
| virtual bool canDereferenceAtCompileTime(TR::SymbolReference *fieldRef, TR::Compilation *comp) { return false; } // safe answer, might change in the future | |||
| virtual bool instanceOfOrCheckCast(J9Class *instanceClass, J9Class* castClass) override; | |||
| virtual bool instanceOfOrCheckCastNoCacheUpdate(J9Class *instanceClass, J9Class* castClass) override; | |||
| virtual bool instanceOfOrCheckCastHelper(J9Class *instanceClass, J9Class* castClass, bool cacheUpdate); | |||
There was a problem hiding this comment.
instanceOfOrCheckCastHelper does not have to be virtual and can be made private because nobody else outside its class needs it.
There was a problem hiding this comment.
Fixed here: 3148b1f
Send a remote message in instanceCheckcast helper outside of the critical section as it's a lengthy operation. Also perform some minor cleanup and make the instanceCheckcast helper a private routine. Signed-off-by: Dhruv Chopra <Dhruv.C.Chopra@ibm.com>
|
Jenkins test sanity.functional+jitaas xlinux jdk8,jdk11 depends eclipse/openj9-omr#jitaas |
|
Only the known |
[skip ci]
Signed-off-by: Dhruv Chopra Dhruv.C.Chopra@ibm.com