Define new IfJitProfiling option for optimizer

[hzongaro]

The list of optimization options includes an IfNotJitProfiling that can be used to run optimizations only if JitProfiling mode is not enabled. This change adds an IfJitProfiling option that can be used to run optimizations only if JitProfiling mode is enabled.

This pull request is part of the fix for the problem reported in issue eclipse-openj9/openj9#15569

Downstream pull request eclipse-openj9/openj9#16155 depends on this pull request.

[hzongaro]

Rahil @r30shah, I'm just running a personal build on this change to make sure there aren't any unexpected test failures, so I've marked it as a draft pull request. Once I mark it as ready-for-review, may I ask you to review this change?

[r30shah]

Hi @hzongaro I did skimmed through the update in eclipse-openj9/openj9#15569 (comment), will go in details soon, but to make clear, One of the main advantage of JProfiling over JIT profiling was that we were not limited in our capabilities in performing optimizations even at profiled very-hot case. So do you think compact locals are not compatible with the trees generated by JProfiling? If not, I would like to fix that.

[hzongaro]

Hi, Rahil @r30shah.

do you think compact locals are not compatible with the trees generated by JProfiling?

No, I think the problem lies with trees that are unrelated to JProfiling. I think that dead trees elimination results in changes to the trees that invalidate the analysis that was performed by compact locals.

Near the end of OpenJ9's hot and very-hot optimization strategies, we find OMR::finalGlobalGroup followed by OMR::profilingGroup. The condition for OMR::profilingGroup is ifProfiling. The finalGlobalOpts includes compactLocals with the condition IfNotJitProfiling. That line has this comment:

analysis results are invalidated by profilingGroup

One of the opts defined in profilingOpts in OpenJ9 is deadTreesElimination. I believe the investigation described in eclipse-openj9/openj9#15569 (comment) highlights one way in which the analysis results produced by compact locals can be invalidated by profiling group.

So if the profiling mode is JitProfiling, compact locals is not run and dead trees elimination might be run. If the profiling mode is JProfiling, compact locals is run and dead trees elimination might be run.

I think we either need to change things so that compact locals is not run if the profiling mode is JProfiling or alternatively, do not run dead trees elimination as part of profilingGroup if the profiling mode is JProfiling.

[r30shah]

Hi @hzongaro sorry for the delay in looking at this. Looking at the detailed update in eclipse-openj9/openj9#15569 (comment) and #6782 (comment) I think the issue is actually with the profilingGroup.
Looking at the list of optimizations that were performed in profilingGroup [1], we use those optimization in the JITProfiling to insert the profiling version of the method body and add control flow at various points to execute the profiling code. I believe deadTreesElimination was added after that to clean up the trees generated in profileGenerator.

In case of JProfiling we would not need to run deadTreesElimination as we would not be doing anything in profileGenerator. Correct fix IMO to run the profilingGroup only when the profiling mode is JITProfiling. Also to avoid confusion, we should rename that as that is specific to JIT profiling.

[1]. https://github.com/eclipse-openj9/openj9/blob/9997d015a7e591cd675c42c01008fc5813ad7682/runtime/compiler/optimizer/J9Optimizer.cpp#L797-L802

[hzongaro]

I was hoping I would be able to rename the branch to reflect the new direction, but Github decided that I had deleted the branch and closed this pull request. I'll reopen it and put my revised changes on the old (now poorly named) branch.

[hzongaro]

Rahil @r30shah, I have revised this pull request along with the downstream OpenJ9 pull request eclipse-openj9/openj9#16155. May I ask you to re-review?

[r30shah]

Looks good to me. @vijaysun-omr Can you please review and merge this change?

[r30shah]

Jenkins build all

[hzongaro]

The linux_ppc-64_le_gcc CI failure appears to be a long-standing problem unrelated to this change. See issue #6571

[vijaysun-omr]

Reviews are done and checks have passed, except a known unrelated problem. Merging.