RFR: 8286104: use aggressive liveness for unstable_if traps

[Vladimir Kozlov]

On Fri, 6 May 2022 19:03:08 GMT, Xin Liu <xliu at openjdk.org> wrote:

> > Are you doing BCI and SP manipulation only to affect result of liveness_at_bci() call in kill_dead_locals()? May it can be done less disruptive way.
> 
> yes, that is my goal. I tried the less disruptive way. One corner case is that the if bytecode may reference an object which happens to be dead at the next bci. It would be wrong if we avoid restoring it in deoptimization. As a result, I have to parse the if-bytecode.

Got it. So the idea is that we don't need to re-materialize Object in case the path which will be taken after deoptimization in Interpreter will not use it. This seems reasonable optimization.

But I am still not sure that current implementation (shift uncommon trap to next bc) is valid.

> 
> Then I came up the idea that we can redefine the semantic of `unstable_if trap`, which is not part of JVM nor Java language specification. I change 'bci' of unstable_if trap from the if branch to the next bci. My assumption is as follows. I think they are both true.
> 
> 1. the original bc has no side-effect.
> 2. reexecution in interpreter takes the other path, or next bci.

First, for your information we re-execute `if` to update its profiling information. For example, if it was some klass check we want to record new value in MDO. That is why we push arguments of `if` back on stack. New value can happen rarely and we may loose it if not recorded during such deoptimization.

I am not sure how your changes verify your assumptions.
And I am not clear what you mean in 2. Re-execution in Interpreter will be done according to information in uncommon trap (bci).

> 
> > I am concern that different bci and sp may affect correctness of uncommon trap call generation (its debug info). It affects result of compute_stack_effects(), too_many_recompiles() and should_reexecute_implied_by_bytecode().
> > In addition logs about such uncommon trap will be different.
> 
> I want to change debuginfo. This [example](https://bugs.openjdk.java.net/browse/JDK-8276998?focusedCommentId=14492303&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14492303) shows that we don't need to save the scalarized object in debuginfo. We don't need to repush arguments to stack(sp) for comparison either.

If you re-execute `if` you need its arguments on stack. I am not sure why you said we don't.

> 
> Both `compute_stack_effects()` and `should_reexecute_implied_by_bytecode()` are called by `GraphKit::add_safepoint_edges()`. we only call it when current bc() is associated with a function call or an allocation. I don't think if-family bytecodes need to call it.

`add_safepoint_edges()` is called for `uncommon_trap` which is generated as runtime call.
I agree that `compute_stack_effects()` may not be called because `must_throw` is false in this case.
But `should_reexecute_implied_by_bytecode()` could be called I think and you may get incorrect answer from `Interpreter::bytecode_should_reexecute(code)` because you changed BCI.

> 
> There are 2 cases in Parse::do_if(). We do use `PreserveJVMState` to save both bci and sp for the taken branch, but we don't use PreserveJVMState for non-taken branch. I am truly concerned about it. so far, my explanation is that we only change bci for unstable_if, so stopped() has been true after that. Besides that, it's the last bytecode of a basic block, parser will reset bci when it processes the next basic block. What do you think about it?

PreserveJVMState is used there only to provide correct starting JVM state for other branch processing. After merge JVM states from both branches are merged.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8545