RFR: 8286104: use aggressive liveness for unstable_if traps

[Xin Liu]

On Thu, 5 May 2022 05:30:06 GMT, Xin Liu <xliu at openjdk.org> wrote:

> I found that some phi nodes are useful because its uses are uncommon_trap nodes. In worse case, it would hinder boxing object eliminating and scalar replacement. Test.java of JDK-8286104 reveals this issue. This patch allows c2 parser to collect liveness based on next bci for unstable_if traps.  In most cases, next bci is closer to exits, so live locals are diminishing.  It helps to reduce the number of inputs of unstable_if traps. 
> 
> This is not a REDO of Optimization of Box nodes in uncommon_trap(JDK-8261137). Two patches are orthogonal.  I adapt test from [TestEliminateBoxInDebugInfo.java](https://github.com/openjdk/jdk/pull/2401/files#diff-49b2e38825aa4c28ca196bdc70c3cbecc2e835c2899f4f393527df4796b177ea), so part of credit goes to the original author.  I found that Scalar replacement can take care of the object `Integer ii = Integer.valueOf(value)` in original test even it can't be removed by later inliner.  I tweak the profiling data of Integer.valueOf() to hinder scalar replacement. 
> 
> This patch can cover the problem discovered by JDK-8261137. I ran the microbench and got 9x speedup on x86_64. It's almost same as JDK-8261137. 
> 
> Before:  
> 
> Benchmark               Mode  Cnt   Score   Error  Units
> MyBenchmark.testMethod  avgt   10  32.776 ± 0.075  us/op
> 
> After: 
> 
> Benchmark               Mode  Cnt   Score   Error  Units
> MyBenchmark.testMethod  avgt   10  3.656 ± 0.006  us/op
>  ```
> 
> Testing
> I ran tier1 test with and without `-XX:+DeoptimizeALot`. No regression has been found yet.

hi, Vladimir and Tobias, 

Thank you for reviewing this patch. 

> 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. 

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.

> 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.  

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. 

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?

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

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