Inliner should use jitInduceOSRAtCurrentPCAndRecompile if recompilation is allowed

[hzongaro]

If TR_InlinerBase::addGuardForVirtual[1] generates a call to induce OSR on the taken side of a virtual call guard, it uses jitInduceOSRAtCurrentPC. Changes to OSR Guard Insertion[2,3] that were made several years ago, however, will generate a call to jitInduceOSRAtCurrentPCAndRecompile if comp()->allowRecompilation() is true. These changes were made under OMR pull request eclipse/omr#3193 and OpenJ9 pull requests #3677 and #3811.

There doesn't seem to be any reason that TR_InlinerBase::addGuardForVirtual should not request recompilation when inducing OSR, if possible. That would allow a method that has inlined a call to another method that has been redefined by HCR to be recompiled, avoiding repeated induction of OSR.

[1] https://github.com/eclipse/omr/blob/9659cbcdcef670fda03609c058e5cb04c58c6511/compiler/optimizer/Inliner.cpp#L2049
[2]

openj9/runtime/compiler/optimizer/OSRGuardInsertion.cpp

Lines 508 to 509 in 5a7404e

	bool induceOSR = comp()->allowRecompilation() ? comp()->getMethodSymbol()->induceOSRAfterAndRecompile(cursor, nodeBCI, guard, false, 0, &cfgEnd):
	comp()->getMethodSymbol()->induceOSRAfter(cursor, nodeBCI, guard, false, 0, &cfgEnd);

[3]

openj9/runtime/compiler/optimizer/OSRGuardInsertion.cpp

Lines 610 to 611 in 5a7404e

	bool induceOSR = comp()->allowRecompilation() ? targetMethod->induceOSRAfterAndRecompile(inductionPoint, nodeBCI, guard, false, comp()->getOSRInductionOffset(cursor->getNode()), &cfgEnd):
	targetMethod->induceOSRAfter(inductionPoint, nodeBCI, guard, false, comp()->getOSRInductionOffset(cursor->getNode()), &cfgEnd);

[gacholio]

Is there any notion of why the OSR is being requested? If it's a guard failure, recomp makes sense. If it's an unexpected cold path that was excluded from compillation, then recomp isn't warranted.

[hzongaro]

Is there any notion of why the OSR is being requested? If it's a guard failure, recomp makes sense. If it's an unexpected cold path that was excluded from compillation, then recomp isn't warranted.

Thanks, @gacholio - I'll need to investigate further

[JamesKingdon]

@jdmpapin made great progress last week on understanding the specific example I'm working on. In that case we start with inlining a method based on profiling data and the compiler later discovers that the type can never be correct. I'm concerned that in this case using induceOSRAfterAndRecompile might make things worse by getting into an infinite cycle of recompilation. Devin found eclipse/omr#6255 which looks like it would have avoided this case, if it weren't for the ifdef on J9VM_OPT_OPENJDK_METHODHANDLE. I only have a very partial understanding of what's going on, but I'm sure Devin can fill in the gaps :)

[jdmpapin]

If it's a guard failure [vs...] an unexpected cold path that was excluded from compilation

I don't think I understand what distinction is being made here. OSR on guard failure is also in place of a cold path that was excluded from compilation

IMO, voluntary OSR is appropriate only in conditions that can be expected with some sufficiently high confidence never to occur. In particular, if we do OSR, that means that we've observed the condition occurring, and so we should arrange to stop doing OSR in that case. That is, we should recompile, and in the new compilation we should avoid at least that particular use of OSR (along with any others that have previously been observed). If we're not going to allow ourselves to recompile after a given compilation, then I think we probably shouldn't use voluntary OSR in that compilation at all

In the case of OSR on guard failure, for many guards we'll already naturally avoid making the same assumption again, e.g. nonoverridden guards. But for a profiled guard, like the one that prompted this issue, it's not obvious that we won't just do the same thing again. After OSR we'll run the call in the interpreter, which I expect will update the profiling, but it might not make a big enough difference in profiling by the time the recompilation is inlining again

Most of the time we'll avoid using OSR on failure of profiled guards, which seems sensible enough. But we can end up using OSR anyway if skipHCRGuardForCallee is true, since in that case we (misleadingly) set createdHCRGuard. This was fixed in eclipse/omr#6255, as James mentioned, but for some reason the fix was restricted to builds that use OpenJDK method handles. I think it would make sense to avoid setting createdHCRGuard in this case in all bulids, e.g.

-   else if (!disableHCRGuards && comp()->getHCRMode() != TR::none)
+   else if (!disableHCRGuards && comp()->getHCRMode() != TR::none && !skipHCRGuardForCallee)
       createdHCRGuard = true;

It might make sense in the future to use OSR on the cold side of some profiled guards, but IMO that would be (a) only for ones we're especially confident about, e.g. interface call sites with a single implementer; and (b) only if we can ensure that we won't repeatedly make the same bet with OSR on recompilation