Do not restore comp phase during comp exception

[mpirvu]

In OMR::Optimizer::optimize() the JIT saves the "compilation phase"
into a stack allocated object of type TR::Compilation::CompilationPhaseScope.
This happens in the constructor of the object. The destructor of the
object restores the previously saved state. This ensures that that if we
leave the scope of the OMR::Optimizer::optimize() routine, the compilation
phase is properly restored.
However, if the optimizer throws an exception, the stack is unwound, the
destructor of the CompilationPhaseScope object is called and the compilation
phase is restored (most likely showing as ILgen). This is not want we want.
For exceptions, we want to retain the compilation phase (namely the optimization)
that produced the exception, so that it can be properly reported at higher
levels, when the exception is caught.
This commit changes the destructor of the CompilationPhaseScope object so that
the compilation phase is restored only if there isn't an exception in progress.
This ensures that the compilation phase at the time when the exception was
thrown is reported correctly.
This commit also moves the comp()->reportOptimizationPhase(optNum);
statement from performOptimization() routine to an earlier location,
as soon as we determine that the current optimization is a proper optimization
and not an optimization group. This ensures that the "analysisPhase" reported
in case of a crash or exception is shown in the context of the current optimization
and not the previous one.

[vijaysun-omr]

I think the main change with this commit may well be around what happens in "IL gen opts". This is an opt strategy that gets run immediately after IL gen, i.e. so it could occur while some other optimization in the strategy is in progress. An example may be if one tries to generate IL during escape analysis (for peeking and/or inlining) and the OOM exception happens in some optimization that is part of IL gen opts.

Which optimization number would we want to report ? The one in IL gen opts or the one in the main optimizer (escape analysis, say) ? There probably isn't a right or wrong answer here, but what do we think is more important ? I could see how gaining information about what the main optimization was when IL gen occurred as being the more interesting answer in some cases. Having said that, I don't recall much confusion in this specific area, and so I may be okay with the change in answer (and simpler code) in this case. Maybe @hzongaro or @jdmpapin have something to share, e.g. if this has been (or can be) an issue.

[mpirvu]

Currently, if the exception happens during ILGen opts, the code will not report ILGen, but report the optimization that was running.

[vijaysun-omr]

By "currently" did you mean before this PR change ? This PR will change it to report the IL gen opt, right ?

[mpirvu]

By "currently" did you mean before this PR change ? This PR will change it to report the IL gen opt, right ?
Yes, this PR will show the optimization as the culprit and not the ILgen phase.

[jdmpapin]

I think Vijay's original question was not about the outermost IL gen phase, but rather IL gen for inlining, which happens during another opt pass (typically the inlining pass)

[mpirvu]

The old and the new implementation report "ILgen" as the current phase only if this is the ILGen of the method being compiled.

[jdmpapin]

The question wasn't about whether to report IL gen phase vs. the optimization from IL gen opts. It was about which optimization we should report when there are two currently running: one "outer" pass running from the main opt strategy (likely inlining, but potentially others, e.g. VP) and one "inner" pass running as part of IL gen opts because the outer pass is inlining something. Vijay seems to be under the impression that the choice of which optimization to report changes in this PR. I haven't read it myself yet, so I can't speak to that - I'm just trying to clear up a seeming miscommunication

As for my two cents, it's not clear to me that one answer is necessarily better than the other in general. Not that it should block the kind of improvement made here, but I think that ideally we should be able to report some kind of nested context, so that in this kind of situation we'd be able to easily tell that e.g. a problem happened during recognized call transformer, during IL gen, during VP

[vijaysun-omr]

Devin's interpretation of what I was asking is correct.

I thought the reported optimization in case of an exception would change as per the comment:
"However, this is not what we want in case an exception is thrown from within OMR::Optimizer::optimize() (or any method called from it). For exceptions, we want to retain the compilation phase (namely the optimization) that produced the exception, so that it can be properly reported at higher levels, when the exception is caught."
in the opening remarks in this PR.

In my later comments, I mentioned that this change in reported optimization in case of an exception (assuming there is a change based on Marius' prior answer) is okay with me, but I wanted to get Henry/Devin opinion too.

[mpirvu]

I ran some experiments with the original code and with the code implemented by this PR.
Case 1: segmentation faults when optimizer (inliner) calls Ilgen which calls optimizer again.
Example of stack trace:

_ZN3OMR9Optimizer19performOptimizationEPK20OptimizationStrategyiii.localalias+0x1a51 (0x000015535AED1371 [libj9jit29.so+0x7f7371])
_ZN3OMR9Optimizer8optimizeEv+0x1ab (0x000015535AED1EEB [libj9jit29.so+0x7f7eeb])
_ZN3OMR20ResolvedMethodSymbol5genILEP11TR_FrontEndPN2TR11CompilationEPNS3_20SymbolReferenceTableERNS3_12IlGenRequestE+0x343 (0x000015535ACDD983 [libj9jit29.so+0x603983])
_ZN23TR_ResolvedJ9MethodBase22_genMethodILForPeekingEPN2TR20ResolvedMethodSymbolEPNS0_11CompilationEbP14TR_PrexArgInfo+0x1fb (0x000015535A91526B [libj9jit29.so+0x23b26b])
_ZN21TR_J9EstimateCodeSize20realEstimateCodeSizeEP13TR_CallTargetP12TR_CallStackbRN2TR6RegionEi+0x997 (0x000015535AAB9BC7 [libj9jit29.so+0x3dfbc7])
_ZN21TR_J9EstimateCodeSize20realEstimateCodeSizeEP13TR_CallTargetP12TR_CallStackbRN2TR6RegionEi+0x1a6b (0x000015535AABAC9B [libj9jit29.so+0x3e0c9b])
_ZN21TR_J9EstimateCodeSize16estimateCodeSizeEP13TR_CallTargetP12TR_CallStackbi.localalias+0x45 (0x000015535AABC575 [libj9jit29.so+0x3e2575])
_ZN19TR_EstimateCodeSize17calculateCodeSizeEP13TR_CallTargetP12TR_CallStackb+0xaf (0x000015535AA261CF [libj9jit29.so+0x34c1cf])
_ZN28TR_MultipleCallTargetInliner13weighCallSiteEP12TR_CallStackP11TR_CallSitebb+0x1fb (0x000015535AAA515B [libj9jit29.so+0x3cb15b])
_ZN28TR_MultipleCallTargetInliner17inlineCallTargetsEPN2TR20ResolvedMethodSymbolEP12TR_CallStackP24TR_InnerPreexistenceInfo+0x1757 (0x000015535AAAE1C7 [libj9jit29.so+0x3d41c7])
_ZN14TR_InlinerBase15performInliningEPN2TR20ResolvedMethodSymbolE+0xae (0x000015535ADBAAAE [libj9jit29.so+0x6e0aae])
_ZN10TR_Inliner7performEv+0x142 (0x000015535AAA4362 [libj9jit29.so+0x3ca362])
_ZN3OMR9Optimizer19performOptimizationEPK20OptimizationStrategyiii.localalias+0x96d (0x000015535AED028D [libj9jit29.so+0x7f628d])
_ZN3OMR9Optimizer8optimizeEv+0x1ab (0x000015535AED1EEB [libj9jit29.so+0x7f7eeb])
_ZN3OMR11Compilation20performOptimizationsEv+0x3e (0x000015535ACC106E [libj9jit29.so+0x5e706e])
_ZN3OMR11Compilation7compileEv+0xa25 (0x000015535ACC2995 [libj9jit29.so+0x5e8995])

Reported vmState is 0x00055cff both with or without this change.
This is decoded as vmState [0x55cff]: {J9VMSTATE_JIT} {methodHandleTransformer} {Performing Optimization}

Case 2: Exception thrown during compilation in the same spot as Case1
Before the change from this PR the VM state printed in vlog is 0x50080 (ILgen).
After the change from this PR the VM state printed in vlog is 0x55cff

[hzongaro]

If I understand correctly, this will ensure that for a vmState of 0x5yyzz, the yy digits will be reported correctly in the verbose log file, but not the zz digits for the analysis phase. I think those will always be FF. To ensure those digits are reportedly correctly I think you would need to make corresponding changes to save and restore the phase before and after the various calls to reportAnalysisPhase. Am I understanding correctly?

[mpirvu]

The zz digits of the analysis phase move linearly through:
BEFORE_OPTIMIZATION
BUILDING_ALIASES (optional, uses CompilationPhaseScope)
BUILDING_STRUCTURE (optional, uses CompilationPhaseScope)
BUILDING_USE_DEFS (optional, uses CompilationPhaseScope)
BUILDING_VALUE_NUMBERS (optional, uses CompilationPhaseScope)
BUILDING_ACCURATE_NODE_COUNT (optional, uses CompilationPhaseScope)
BUILDING_FREQUENCIES (optional, uses CompilationPhaseScope)
PERFORMING_OPTIMIZATION (the encoding of this one is 0xff, most common value to see in practice)
AFTER_OPTIMIZATION

For the phases that use a CompilationPhaseScope a compilation exception will restore the zz digits to "BEFORE_OPTIMIZATION".
If desirable, I can eliminate the CompilationPhaseScope for all such analyses.

[hzongaro]

PERFORMING_OPTIMIZATION (the encoding of this one is 0xff, most common value to see in practice)

Yes, but others do occur, which is why I recently added support to the -Xjit:vmState option to decode the other analysis phase values.

If desirable, I can eliminate the CompilationPhaseScope for all such analyses.

I think it would be good to accurately capture the state in the verbose log for both the optimization and the analysis phase, as it can help to narrow down problems. I don't have a suggestion off-hand for a better way of doing it than eliminating CompilationPhaseScope more broadly.

[mpirvu]

I think it would be good to accurately capture the state in the verbose log for both the optimization and the analysis phase

I adjusted the code to eliminate the usage of CompilationPhaseScope. The Vmstate now shows both the optimization and the analysis phase. E.g.:

! (warm) java/util/ImmutableCollections$SetN.probe(Ljava/lang/Object;)I Q_SZ=0 Q_SZI=0 QW=12 j9m=00007FF13C1565E8 time=1275us compilationExcessiveComplexity VmState=0x000504aa OptIdx=16 OptName=CFGSimplification memLimit=262144 KB freePhysicalMemory=12948 MB mem=[region=832 system=16384]KB compThreadID=0

[vijaysun-omr]

I'll wait for approval from @hzongaro and @jdmpapin (or the discussion to end) before starting tests.

[hzongaro]

I think the change does what was intended. If these are the only remaining uses of TR::Compilation::CompilationPhaseScope, I think the declaration and implementation of the class can be removed from OMRCompilation.hpp and OMRCompilation.cpp.

[jdmpapin]

I didn't spot any break, continue, return, etc. between the saves and the restores, but maybe we could make it more obvious that the saved state will be restored on any non-exceptional exit path if we keep using CompilationPhaseScope but restrict the effect of the destructor to the case where !std::uncaught_exception()

[mpirvu]

I see that uncaught_exception() is deprecated in C++17 and eliminated in C++20. We are not there yet, but we may have to change the code in the future. Probably not very soon though.

[jdmpapin]

It seems like the eventual change would be pretty trivial, just !std::uncaught_exception() to std::uncaught_exceptions() == 0, though maybe there would be a transition period where we need to use one or the other depending on the build compiler. I see that we already have two other uses of std::uncaught_exception() that will eventually need to be updated or otherwise dealt with, so I think another one wouldn't cause much of an increase in the amount of work required for that.

That said, I'll leave it to your judgment. If you want to keep this change as it is, then I agree with Henry that CompilationPhaseScope can be deleted

[mpirvu]

I will attempt to use std::uncaught_exception(). I was worried about the possibility of not being able to compile on all platforms, but if it's already being used in omr, then it's fair game.
There is this piece of code which is excluded on AIX for some reason:

TR_CallStack::~TR_CallStack()
{
    //::commit is supposed to clear up the lists after everything has been propagated to a caller
    // if there are still some residual symRefs left it means that we missed a call to commit somewhere
#if !defined(AIXPPC)
    TR_ASSERT(std::uncaught_exception() || (_autos.isEmpty() && _temps.isEmpty() && _injectedBasicBlockTemps.isEmpty()),
        "lists should have been emptied by TR_CallStack::commit");
#endif
}

I wonder whether there is something wrong with std::uncaught_exception() on AIX.

[hzongaro]

I wonder whether there is something wrong with std::uncaught_exception() on AIX.

The change that introduced #if !defined(AIXPPC) (in 2015) doesn't mention why it was needed.

[mpirvu]

Personal builds with code from this PR have passed (aarch64_linux, aarch64_mac, ppc64_aix, ppcle_linux, s390x_linux, x86-64_linux, x86-64_mac, x86-64_windows)

[hzongaro]

Looks good. Thanks!

[vijaysun-omr]

Jenkins build all

[vijaysun-omr]

Jenkins build riscv

[mpirvu]

The error on linux_risc64 (https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64/416):
17:11:53 28/29 Test #28: comptest ..........................***Exception: Illegal203.11 sec
has been seen before, for example here: https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64/403/consoleFull triggered from #7933

The error on linux_ppc-64_le_gcc has been seen many times before, most recently here: https://ci.eclipse.org/omr/job/PullRequest-linux_ppc-64_le_gcc/4886/