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

[Vladimir Kozlov]

On Wed, 1 Jun 2022 19:06:18 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 one additional commit since the last revision:
> 
>   Remove useless flag. if jdwp is on, liveness_at_bci() marks all local
>   variables live.

Few comments.

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

> 606:                   _expensive_nodes   (comp_arena(), 8, 0, NULL),
> 607:                   _for_post_loop_igvn(comp_arena(), 8, 0, NULL),
> 608:                   _unstable_ifs      (comp_arena(), 8, 0, NULL),

I think `unstable_ifs` should be `unstable_if_traps` here and in all related method names and variables. You record traps now.

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

> 774: 
> 775:     preprocess_unstable_ifs();
> 776: 

Why  call preprocessing here and not after `PhaseRemoveUseless` which can remove some paths?

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

> 1862: }
> 1863: 
> 1864: void Compile::invalidate_unstable_if(CallStaticJavaNode* unc) {

Add description comment for this method. What it is used for.

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

> 1875: uint unstable_ifs_all              = 0;
> 1876: 
> 1877: void Compile::preprocess_unstable_ifs() {

I assume it will be used in product VM  by next changes 8287385.
Otherwise counters and method should be under `#ifndef PRODUCT` because they used only for statistics.

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

> 1897:     int next_bci = trap->next_bci();
> 1898: 
> 1899:     if (next_bci != -1 && !_dead_node_list.test(unc->_idx)) {

Did you consider to remove items from `_unstable_ifs` in `Compile::remove_useless_nodes()` to avoid `_dead_node_list` check here?
You would need specialized `remove_useless__unstable_ifs(useful)`

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

> 609: class UnstableIfTrap {
> 610:   CallStaticJavaNode* _unc;
> 611:   Parse::Block* _path;  // the pruned path

`const` for these 2 fields?

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

> 633:   // or if _path has more than one predecessor and has been parsed, _unc does not mask out any real code.
> 634:   bool is_trivial() const {
> 635:     return _path->is_parsed();

Should you also check `_next_bci != -1` ?

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

> 646:   inline void* operator new(size_t x) throw() {
> 647:     Compile* C = Compile::current();
> 648:     return C->node_arena()->AmallocWords(x);

Use `comp_arena`, the same place where `_unstable_ifs` is allocated.

src/hotspot/share/opto/parse1.cpp line 98:

> 96: 
> 97:   if (unstable_ifs_all) {
> 98:     tty->print_cr("%u trivial unstable_ifs (%2d%%)", trivial_unstable_ifs,

"trivial unstable_ifs traps"

test/hotspot/jtreg/compiler/c2/irTests/TestAggressiveLivenessForUnstableIf.java line 2:

> 1: /*
> 2:  * Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.

Missing year.

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

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