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

[Roberto Castañeda Lozano]

On Wed, 21 Oct 2020 17:49:24 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

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

These two runs (`stress1` and `stress2`) test that compilation time does not explode. When it does (before the proposed fix), the runs take > 10s to execute (but less than 120s, at least `stress1`), while after the fix both take just a few ms (all measurements from my local and our CI machines). I just deemed 1s to be a good value to balance the risk of false positives (reporting an nonexistent bug because the timeout is too short) and false negatives (not reporting a real bug because the timeout is too long). I will explore further increasing the load of these runs to see if we can increase the timeout without increasing the risk of false negatives.

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

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