RFR: 8286104: use aggressive liveness for unstable_if traps [v2]

[Vladimir Kozlov]

On Thu, 12 May 2022 21:27:30 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.
>
> Xin Liu has updated the pull request incrementally with 11 additional commits since the last revision:
> 
>  - revert code change from 1st revision.
>  - Merge branch 'JDK-8276998' into JDK-8286104
>  - rule out if a If nodes has 2 branches of unstable_if trap.
>  - change the flag to diagnostic.
>  - add sanity check for operands if bc is if_acmp_eq/ne and ifnull/nonnull
>  - fix release build
>  - update unstable_if after igvn.
>  - adjust unstable_if after fold_compares
>  - disable comparison_folding temporarily.
>    
>    This feature not only folds two CMPI but also merge two uncommon_traps.
>    it uses the dominating uncommon_trap and revaluate the two if in
>    interpreter.  currently, aggressiveliveness can't work for that.
>  - retain bci for unstable_if
>  - ... and 1 more: https://git.openjdk.java.net/jdk/compare/2c38b87b...2f047457

We can work with this approach.
Did you consider to record uncommon traps in `adjust_map_after_if()` instead of `If` node in its `Ideal()` method? And using new `CallStaticJavaNode::_unc_bci` instead of one in `IfNode`.


I am suggesting it because it looks logical to track uncommon traps instead of Ifs for this optimization. And `process_for_unstable_ifs()` mostly process information from uncommon traps. I am not sure why you decided to record `IfNode`. May be it was much simpler way when you need to reset them when uncommon traps are merged in two places you do `set_unc_bci()`. But you can do the same for uncommon traps there.
You need also make sure uncommon trpa call nodes are removed from the list when they are removed from code.

Also `uncomon_trap_proj()` may return merged uncommon trap (referenced through Region node). Instead you can check if it simple `If->Proj->unc_trap` pattern to filter out complex cases.

You call `process_for_unstable_ifs` in 2 places. I understand why you want to do that before `inline_boxing_calls()`. But why before `inline_incrementally()`? Why you need second call?

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

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