RFR: 8254317: C2: Resource consumption of ConvI2LNode::Ideal() grows exponentially [v2]

[Vladimir Kozlov]

On Wed, 21 Oct 2020 10:04:14 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> Prevent exponential number of calls to `ConvI2LNode::Ideal()` when AddIs are used multiple times by other AddIs in the optimization ConvI2L(AddI(x, y)) -> AddL(ConvI2L(x), ConvI2L(y)). This is achieved by (1) reusing existing ConvI2Ls if possible rather than eagerly creating new ones and (2) postponing the optimization of newly created ConvI2Ls. Remove hook node solution introduced in [8217359](https://github.com/openjdk/jdk/commit/cf554816d1952f722143e9d03ec669e80f955adf), since this is subsumed by (2). Use `phase->is_IterGVN()` rather than `can_reshape` to check if `ConvI2LNode::Ideal()` is called within iterative GVN, for clarity. Add regression tests that cover different shapes and sizes of AddI subgraphs, implicitly checking (by not timing out) that there is no combinatorial explosion.
>
> Roberto Castañeda Lozano has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision:
> 
>  - Merge master
>  - Generalize the fix to handle any input where AddIs are used multiple times by
>    other AddIs, which could also lead to an exponential number of calls to
>    ConvI2LNode::Ideal(). This is achieved by (1) reusing existing ConvI2Ls if
>    possible rather than eagerly creating new ones and (2) postponing the
>    optimization of newly created ConvI2Ls. Remove "hook" node solution introduced
>    in JDK-8217359 since this is subsumed by (2). Test that ConvI2LNode::Ideal() is
>    called within iterative GVN using phase->is_IterGVN() rather than can_reshape,
>    for clarity.
>    
>    Merge all tests into a single class. Reimplement the microbenchmark as a test
>    case that should time out in case of a combinatorial explosion. Add a second
>    similar microbenchmark that demonstrates the need for this generalization.
>  - Merge master
>  - 8254317: C2: Resource consumption of ConvI2LNode::Ideal() grows exponentially
>    
>    In the optimization ConvI2L(AddI(x, y)) -> AddL(ConvI2L(x), ConvI2L(y)) within
>    ConvI2LNode::Ideal(), handle the special case x = y by feeding both inputs of
>    AddL from a single ConvI2L node rather than creating two semantically equivalent
>    ConvI2L nodes. This avoids an exponential number of calls to
>    ConvI2LNode::Ideal() when dealing with long chains of AddI nodes. Disable the
>    optimization for the pattern ConvI2L(SubI(x, x)), which is simplified to zero
>    during parsing anyway. Add a set of regression tests for the transformation that
>    cover different shapes of AddI subgraphs. Also add a microbenchmark that
>    exercises the special case, for performance regression testing.

Changes requested by kvn (Reviewer).

test/hotspot/jtreg/compiler/conversions/TestMoveConvI2LThroughAddIs.java line 41:

> 39:  *      -XX:CompileOnly=::testChain,::testTree,::testDAG
> 40:  *      compiler.conversions.TestMoveConvI2LThroughAddIs basic
> 41:  * @run main/othervm/timeout=1 -Xcomp -XX:-TieredCompilation -XX:-Inline

By default timeout is 120 sec. Why you set it to 1 sec?
The same for second @run.

test/hotspot/jtreg/compiler/conversions/TestMoveConvI2LThroughAddIs.java line 38:

> 36:  *          the short specified timeout.
> 37:  * @library /test/lib /
> 38:  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:-Inline

For future tests writing.
In general we should avoid using -Xcomp because it does not provide profiling information to C2 and it may generate unexpected code because branches frequencies are not known.
Preferable way is warm up test method by calling it in a loop enough times to make sure it is compiled and then verify results outside test method.
Then you don't need -XX:CompileOnly command.

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

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