RFR: 8315916: assert(C->live_nodes() <= C->max_node_limit()) failed: Live Node limit exceeded [v6]

[Christian Hagedorn]

On Thu, 24 Oct 2024 04:14:47 GMT, Dhamoder Nalla <dhanalla at openjdk.org> wrote:

>> In the debug build, the assert is triggered during the parsing (before Code_Gen). In the Release build, however, the compilation bails out at `Compile::check_node_count()` during the code generation phase and completes execution without any issues.
>> 
>> When I commented out the assert(C->live_nodes() <= C->max_node_limit()), both the debug and release builds exhibited the same behavior: the compilation bails out during code_gen after building the ideal graph with more than 80K nodes.
>> 
>> The proposed fix will check the live node count and bail out during compilation while building the graph for scalarization of the elements in the array when the live node count crosses the limit of 80K, instead of unnecessarily building the entire graph and bailing out in code_gen.
>
> Dhamoder Nalla has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix trailing whitespace

src/hotspot/share/opto/escape.hpp line 680:

> 678:   bool add_final_edges_unsafe_access(Node* n, uint opcode);
> 679: 
> 680:   int invocation() { return _invocation; }

Can be made `const`:
Suggestion:

  int invocation() const { return _invocation; }

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 33:

> 31:  */
> 32: 
> 33: package compiler.loopopts.superword;

I suggest to move this test to `compiler/escapeAnalysis` and update the package accordingly to `compiler.escapeAnalysis`.

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 35:

> 33: package compiler.loopopts.superword;
> 34: 
> 35: public class TestScalarize_Bailout {

You should not use underlines in class names

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 37:

> 35: public class TestScalarize_Bailout {
> 36: 
> 37: static Object var1;

Indentation seems to be off.

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 43:

> 41:     try {
> 42:         Class <?> Class37 = Class.forName("compiler.loopopts.superword.TestScalarize_Bailout");
> 43:         synchronized (compiler.loopopts.superword.TestScalarize_Bailout.class) {

I guess you do not need the fully qualified name:
Suggestion:

        synchronized (TestScalarize_Bailout.class) {

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 43:

> 41:     try {
> 42:         Class <?> Class37 = Class.forName("compiler.loopopts.superword.TestScalarize_Bailout");
> 43:         synchronized (compiler.loopopts.superword.TestScalarize_Bailout.class) {

Is `forName()` and `synchronized` really required to trigger this with mainline? If so, you should add a comment to explain why this is required.

test/hotspot/jtreg/compiler/loopopts/superword/TestScalarize_Bailout.java line 48:

> 46:           }
> 47:         }
> 48:     } catch (Exception eeeeeeee){throw new RuntimeException(eeeeeeee);}

Suggestion:

    } catch (Exception e) {
        throw new RuntimeException(e);
    }

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827660522
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827664200
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827662890
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827739836
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827665705
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827741280
PR Review Comment: https://git.openjdk.org/jdk/pull/20504#discussion_r1827666392