Skip to content

Commit

Permalink
Merge pull request #16122 from jdmpapin/iface-inl
Browse files Browse the repository at this point in the history 
Disallow nop guards for interface call sites
  • Loading branch information
@pshipton
pshipton authored Oct 19, 2022
2 parents 914a3ed + 6dbf777 commit 0d7572f
Show file tree
Hide file tree
Showing 3 changed files with 266 additions and 8 deletions.
8 changes: 6 additions & 2 deletions runtime/compiler/optimizer/InlinerTempForJ9.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2855,7 +2855,7 @@ TR_MultipleCallTargetInliner::eliminateTailRecursion(
backEdge = TR::CFGEdge::createEdge(gotoBlock, branchDestination, trMemory());
callerCFG->addEdge(backEdge);
callerCFG->removeEdge(origEdge);
if (guard->_kind == TR_ProfiledGuard)
if (guard->_kind == TR_ProfiledGuard && !guard->_forceTakenSideCold)
{
if (block->getFrequency() < 0)
block2->setFrequency(block->getFrequency());
Expand Down Expand Up @@ -6818,7 +6818,11 @@ TR_J9InlinerPolicy::suitableForRemat(TR::Compilation *comp, TR::Node *callNode,

bool suitableForRemat = true;
TR_AddressInfo *valueInfo = static_cast<TR_AddressInfo*>(TR_ValueProfileInfoManager::getProfiledValueInfo(callNode, comp, AddressInfo));
if (guard->isHighProbablityProfiledGuard())
if (guard->_forceTakenSideCold)
{
// remat ok
}
else if (guard->isHighProbablityProfiledGuard())
{
if (comp->getMethodHotness() <= warm && comp->getPersistentInfo()->getJitState() == STARTUP_STATE)
{
Expand Down
3 changes: 2 additions & 1 deletion runtime/compiler/optimizer/J9CallGraph.hpp
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2021 IBM Corp. and others
* Copyright (c) 2000, 2022 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -87,6 +87,7 @@ class TR_J9InterfaceCallSite : public TR_ProfileableCallSite
public:
TR_CALLSITE_TR_ALLOC_AND_INHERIT_EMPTY_CONSTRUCTOR(TR_J9InterfaceCallSite, TR_ProfileableCallSite)
virtual bool findCallSiteTarget (TR_CallStack *callStack, TR_InlinerBase* inliner);
bool findCallSiteTargetImpl (TR_CallStack *callStack, TR_InlinerBase* inliner);
virtual TR_OpaqueClassBlock* getClassFromMethod ();
virtual const char* name () { return "TR_J9InterfaceCallSite"; }
protected:
Expand Down
263 changes: 258 additions & 5 deletions runtime/compiler/optimizer/J9Inliner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,108 @@ static TR_ResolvedMethod * findSingleImplementer(
*/

bool TR_J9InterfaceCallSite::findCallSiteTarget (TR_CallStack *callStack, TR_InlinerBase* inliner)
{
bool result = findCallSiteTargetImpl(callStack, inliner);

// A passing vgnop-based interface guard can guarantee the receiver type is
// as expected by the inlined body, but only if we already know before the
// guard that the receiver implements the expected interface. Here, an
// interface type bound is insufficient to know that, because bytecode
// verification does not guarantee any particular receiver type at
// invokeinterface.
//
// As such, in the absence of a class (i.e. non-interface) type bound on the
// receiver, all targets must use profiled guard. Additionally, the profiled
// guard must ensure on the hot side that the receiver is an instance of the
// interface expected at this call site.
//
// These requirements could be relaxed in the future by generating a type
// check against the interface type at the beginning of the inlined body
// (taking care to ensure that the exception successors are the same as
// those of the call, rather than those of the first block of the callee).
//
// ZEROCHK jitThrowIncompatibleClassChangeError
// instanceof
// <receiver>
// loadaddr <interface>
//
// In particular, such a type check would allow the following cases:
//
// - A default method inlined using a nonoverridden guard. Currently,
// default methods are never inlined (at interface call sites) because
// TR_ResolvedJ9Method::getResolvedInterfaceMethod() does not return them.
//
// - A profiled guard with method test for a method defined by a class that
// does not implement the expected interface. Subtypes may still implement
// the interface.
//
// If/when we want to start inlining in one or both of these cases, the
// assertions below can be relaxed accordingly. However, testing instanceof
// against the interface will be expensive, so there would have to be a
// considerable benefit to the inlining to motivate such a change.
//
TR_OpaqueClassBlock *iface = getClassFromMethod();
if (_receiverClass != NULL
&& !TR::Compiler->cls.isInterfaceClass(comp(), _receiverClass))
{
TR_ASSERT_FATAL(
fe()->isInstanceOf(_receiverClass, iface, true, true, true) == TR_yes,
"interface call site %p receiver type %p "
"does not implement the expected interface %p",
this,
_receiverClass,
iface);

heuristicTrace(
inliner->tracer(),
"Interface call site %p has receiver class bound %p; nop guards ok",
this,
_receiverClass);
}
else
{
TR_Debug *debug = comp()->getDebug();
for (int32_t i = 0; i < numTargets(); i++)
{
TR_CallTarget *tgt = getTarget(i);
TR_VirtualGuardKind kind = tgt->_guard->_kind;
TR_ASSERT_FATAL(
kind == TR_ProfiledGuard,
"interface call site %p requires profiled guard (kind %d), "
"but target %d [%p] uses %s (kind %d)",
this,
(int)TR_ProfiledGuard,
i,
tgt,
debug == NULL ? "<unknown name>" : debug->getVirtualGuardKindName(kind),
(int)kind);

// Bound on the receiver types that pass the profiled guard
TR_OpaqueClassBlock *passClass = NULL;
TR_ResolvedMethod *callee = tgt->_calleeMethod;
if (tgt->_guard->_type == TR_VftTest)
passClass = tgt->_receiverClass;
else
passClass = callee->containingClass();

TR_ASSERT_FATAL(
fe()->isInstanceOf(passClass, iface, true, true, true) == TR_yes,
"interface call site %p target %d [%p] (J9Method %p) "
"accepts receivers of type %p, "
"which does not implement the expected interface %p",
this,
i,
tgt,
callee->getPersistentIdentifier(),
passClass,
iface);
}
}

return result;
}

bool TR_J9InterfaceCallSite::findCallSiteTargetImpl(TR_CallStack *callStack, TR_InlinerBase* inliner)
{
static char *minimizedInlineJIT = feGetEnv("TR_JITInlineMinimized");

Expand Down Expand Up @@ -733,10 +835,152 @@ bool TR_J9InterfaceCallSite::findCallSiteTarget (TR_CallStack *callStack, TR_Inl

if (calleeResolvedMethod && !calleeResolvedMethod->virtualMethodIsOverridden())
{
TR_VirtualGuardSelection * guard = new (comp()->trHeapMemory()) TR_VirtualGuardSelection(TR_InterfaceGuard, TR_MethodTest);
addTarget(comp()->trMemory(),inliner,guard,calleeResolvedMethod,_receiverClass,heapAlloc);
heuristicTrace(inliner->tracer(),"Call is an Interface with a Single Implementer guard %p\n", guard);
return true;
TR_VirtualGuardKind kind = TR_ProfiledGuard;
TR_VirtualGuardTestType testType = TR_DummyTest;
TR_OpaqueClassBlock *thisClass = _receiverClass;
if (_receiverClass != NULL
&& !TR::Compiler->cls.isInterfaceClass(comp(), _receiverClass))
{
kind = TR_InterfaceGuard;
testType = TR_MethodTest;
}
else
{
// Whether to try to choose VFT test based on a non-extended defClass
// or based on profiling. This does not affect the final defClass
// heuristic because in that case we can be certain that the class
// won't be extended later.
bool useVftTestHeuristics = true;
TR::PersistentInfo *persistInfo = comp()->getPersistentInfo();
if (TR::Compiler->vm.isVMInStartupPhase(comp()->fej9()->getJ9JITConfig()))
{
const static bool useVftTestHeuristicsDuringStartup =
feGetEnv("TR_useInterfaceVftTestHeuristicsDuringStartup") != NULL;

useVftTestHeuristics = useVftTestHeuristicsDuringStartup;
}

// Profiled guards must guarantee that passing receivers are instances
// of some class that implements the expected interface. See the
// comment in TR_J9InterfaceCallSite::findCallSiteTarget().
//
// The choice of VFT test vs. method test is irrelevant here, as
// either one would accept instances of defClass. However, a VFT test
// obtained by consulting the profiled receiver types would work.
//
TR_OpaqueClassBlock *defClass = calleeResolvedMethod->containingClass();
thisClass = defClass;
TR_OpaqueClassBlock *iface = getClassFromMethod();
if (fe()->isInstanceOf(defClass, iface, true, true, true) != TR_yes)
{
calleeResolvedMethod = NULL; // hope to get a VFT test from profiling
}
// Heuristically choose between VFT test and method test. VFT test
// is cheaper, but method test can potentially allow the inlined
// body to be run for more receivers.
else if (TR::Compiler->cls.isClassFinal(comp(), defClass))
{
testType = TR_VftTest; // method test will never help
}
else if (useVftTestHeuristics && !fe()->classHasBeenExtended(defClass))
{
// Hope that defClass won't be extended in the future, or if it is,
// that its subtypes will override the inlined method anyway.
testType = TR_VftTest;
}
else
{
// There's already at least one subclass inheriting the single
// implementation. Choose method test because it covers the
// defining class and (so far) all of its subclasses. (If there
// were a subclass with its own override, then calleeResolvedMethod
// would be overridden.)
testType = TR_MethodTest;

// Still consult the profiling though, since it might reveal that
// one type is overwhelmingly frequent at this call site. In that
// case, change back to VFT test.
TR_ValueProfileInfoManager *profMgr =
TR_ValueProfileInfoManager::get(comp());

TR_AddressInfo *valueInfo = NULL;
if (profMgr != NULL)
{
valueInfo = static_cast<TR_AddressInfo*>(
profMgr->getValueInfo(_bcInfo, comp(), AddressInfo));
}

if (useVftTestHeuristics
&& valueInfo != NULL
&& !comp()->getOption(TR_DisableProfiledInlining))
{
TR_ScratchList<TR_ExtraAddressInfo> byFreqDesc(comp()->trMemory());
valueInfo->getSortedList(comp(), &byFreqDesc);
ListIterator<TR_ExtraAddressInfo> it(&byFreqDesc);
uint32_t remainingTotalFreq = valueInfo->getTotalFrequency();
TR_OpaqueClassBlock *topProfiledClass = NULL;
uint32_t topProfiledFreq = 0;
TR_ExtraAddressInfo *cur = it.getFirst();
for (; cur != NULL; cur = it.getNext())
{
auto *curClass =
reinterpret_cast<TR_OpaqueClassBlock*>(cur->_value);

if (persistInfo->isObsoleteClass(curClass, comp()->fe())
|| fe()->isInstanceOf(curClass, iface, true, true, true) != TR_yes)
{
remainingTotalFreq -= cur->_frequency;
}
else if (topProfiledClass == NULL)
{
topProfiledClass = curClass;
topProfiledFreq = cur->_frequency;
}
}

if (topProfiledClass != NULL
&& remainingTotalFreq >= 32
&& topProfiledFreq == remainingTotalFreq)
{
testType = TR_VftTest;
thisClass = topProfiledClass;
}
}
}
}

if (calleeResolvedMethod != NULL)
{
TR_ASSERT_FATAL(testType != TR_DummyTest, "failed to select a guard test type");
TR_VirtualGuardSelection *guard =
new (comp()->trHeapMemory()) TR_VirtualGuardSelection(
kind, testType, thisClass);

if (kind == TR_ProfiledGuard)
{
// Almost all bytecode would pass type checking even including
// interface types. So even though this can't be a nop guard
// (because verification doesn't check interface types), treat it
// as much like a nop guard as possible. In particular, this will
// ensure that the block containing the cold call is actually
// marked cold, and ensure that priv. arg remat is allowed.
guard->_forceTakenSideCold = true;

// It is still a high-probability profiled guard...
guard->setIsHighProbablityProfiledGuard();
}

addTarget(
comp()->trMemory(),
inliner,
guard,
calleeResolvedMethod,
thisClass,
heapAlloc);

heuristicTrace(inliner->tracer(),"Call is an Interface with a Single Implementer guard %p\n", guard);
return true;
}
}

return findProfiledCallTargets(callStack, inliner);
Expand Down Expand Up @@ -1052,7 +1296,16 @@ void TR_ProfileableCallSite::findSingleProfiledReceiver(ListIterator<TR_ExtraAdd
continue;
}

//origMethod
if (preferMethodTest && isInterface())
{
// For interface call sites, the profiled guard must allow only
// receivers that are instances of the expected interface. See the
// comment in TR_J9InterfaceCallSite::findCallSiteTarget().
TR_OpaqueClassBlock *defClass = targetMethod->containingClass();
TR_OpaqueClassBlock *iface = getClassFromMethod();
if (fe()->isInstanceOf(defClass, iface, true, true, true) != TR_yes)
preferMethodTest = false;
}

TR_J9VMBase *fej9 = (TR_J9VMBase *)(comp()->fe());
// need to be able to store class chains for these methods
Expand Down

0 comments on commit 0d7572f

Please sign in to comment.