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

[Tobias Hartmann]

On Mon, 6 Jun 2022 20:42:22 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://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/2401/files*diff-49b2e38825aa4c28ca196bdc70c3cbecc2e835c2899f4f393527df4796b177ea__;Iw!!ACWV5N9M2RV99hQ!LBkYuxRT3i7yjqe-yTfuJfpv7Mr-Jt-kEMHUHsWt9TCrYawzpPy2KOpBjIu5brE93J935ys3MGr69j87NHBvnWARACQsh_YURg$ ), 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 one additional commit since the last revision:
> 
>   monior change for code style.

Looks good overall. Some comments/questions:
- Why can't we remove traps that have been modified?
- I'm wondering how useful `Compile::print_statistics()` really is. Is it worth extending it? Is anyone using it?
- Do you need to check for unstable if traps in `Node::destruct`?

src/hotspot/share/opto/compile.cpp line 1929:

> 1927:     int next_bci = trap->next_bci();
> 1928: 
> 1929:     if (next_bci != -1 && !trap->modified()) {

How can it be already modified? We are only processing each trap once, right?

src/hotspot/share/opto/ifnode.cpp line 842:

> 840:       if (!igvn->C->too_many_traps(dom_method, dom_bci, Deoptimization::Reason_unstable_fused_if) &&
> 841:           !igvn->C->too_many_traps(dom_method, dom_bci, Deoptimization::Reason_range_check) &&
> 842:           igvn->C->remove_unstable_if_trap(dom_unc)) {

This should be moved to `IfNode::merge_uncommon_traps`.

src/hotspot/share/opto/parse.hpp line 607:

> 605: 
> 606: // Specialized uncommon_trap of unstable_if, we have 2 optimizations for them:
> 607: //   1. suppress trivial Unstable_If traps

Where is this done?

src/hotspot/share/opto/parse.hpp line 609:

> 607: //   1. suppress trivial Unstable_If traps
> 608: //   2. use next_bci of _path to update live locals.
> 609: class UnstableIfTrap {

What about moving this information into `CallStaticJavaNode`?

src/hotspot/share/opto/parse.hpp line 622:

> 620:   }
> 621: 
> 622:   // The starting point of the pruned block, where control should go

Suggestion:

  // The starting point of the pruned block, where control goes

src/hotspot/share/opto/parse.hpp line 636:

> 634:   }
> 635: 
> 636:   Parse::Block* path() const {

This method is not used.

src/hotspot/share/opto/parse.hpp line 643:

> 641:   // if _path has only one predecessor, it is trivial if this block is small(1~2 bytecodes)
> 642:   // or if _path has more than one predecessor and has been parsed, _unc does not mask out any real code.
> 643:   bool is_trivial() const {

But these properties are not checked by the method, right?

Also, the code is only used in debug, should it be guarded?

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

Changes requested by thartmann (Reviewer).

PR: https://git.openjdk.org/jdk/pull/8545