RFR: 8349523: Unused runtime calls to drem/frem should be removed [v7]

[Christian Hagedorn]

On Fri, 28 Feb 2025 15:09:07 GMT, Marc Chevalier <duke at openjdk.org> wrote:

>> Remove frem and drem macros nodes when the result is not used. These nodes have other outputs (like memory), which is not meaningful, but preventing them to be dropped so easily. This patch removes the useless frem/drem nodes, and by rewiring the inputs to the outputs.
>> 
>> Thanks,
>> Marc
>
> Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision:
> 
>   more precise testing

Nice test updates! Some final comments, then I think it's good to go from my side :)

test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java line 151:

> 149:     // is that they exercise a slightly different reason why the node is being removed,
> 150:     // and thus a different execution path. In unusedResultAfterLoopOpt1 the modulo is
> 151:     // used in the traps of the parse predicate. In unusedResultAfterLoopOpt2, it is not.

Suggestion:

    // used in the traps of the parse predicates. In unusedResultAfterLoopOpt2, it is not.

test/hotspot/jtreg/compiler/c2/irTests/ModDNodeTests.java line 153:

> 151:     // used in the traps of the parse predicate. In unusedResultAfterLoopOpt2, it is not.
> 152:     @Test
> 153:     @IR(failOn = {"drem"}, phase = CompilePhase.BEFORE_MATCHING)

This is not required now anymore since you do the matching on the node directly instead of the expanded call. Same for the other rules below.

test/hotspot/jtreg/compiler/c2/irTests/ModFNodeTests.java line 151:

> 149:     // is that they exercise a slightly different reason why the node is being removed,
> 150:     // and thus a different execution path. In unusedResultAfterLoopOpt1 the modulo is
> 151:     // used in the traps of the parse predicate. In unusedResultAfterLoopOpt2, it is not.

Suggestion:

    // used in the traps of the parse predicates. In unusedResultAfterLoopOpt2, it is not.

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 2596:

> 2594:         IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.BEFORE_MACRO_EXPANSION, regex,
> 2595:                 CompilePhase.BEFORE_STRINGOPTS,
> 2596:                 CompilePhase.BEFORE_MACRO_EXPANSION));

Indentation:
Suggestion:

        IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.BEFORE_MACRO_EXPANSION, regex,
                                                                          CompilePhase.BEFORE_STRINGOPTS,
                                                                          CompilePhase.BEFORE_MACRO_EXPANSION));

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

PR Review: https://git.openjdk.org/jdk/pull/23694#pullrequestreview-2651134729
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1975632096
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1975633790
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1975634535
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1975637962