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

[Christian Hagedorn]

On Tue, 25 Feb 2025 21:13:36 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:
> 
>   Factor testing whether a node is a data proj of a pure function

Update looks good. One more suggestion for the IR tests.

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

> 122: 
> 123:     @Test
> 124:     @IR(failOn = {"drem"}, phase = CompilePhase.BEFORE_MATCHING)

I thought about this again and I would generally be as precise as possible and do the matching on the first `CompilePhase` where we expect the node to be removed (if such a unique `CompilePhase` exists or pick one that is as early as possible and is unique). Moreover, it would be good to have a separate `IRNode` entry for `ModF/D`. I skimmed through the `IRNode` file and it seems that we miss a `macroNodes()` specific method that makes sure we only match macro nodes on the corresponding `CompilePhases`. You could add the following to `IRNode.java` (untested):


    public static final String MOD_F = PREFIX + "MOD_F" + POSTFIX;
    static {
        macroNodes(MOD_F, "ModF");
    }

    public static final String MOD_D = PREFIX + "MOD_D" + POSTFIX;
    static {
        macroNodes(MOD_D, "ModD");
    }

   ...

    /**
     * Apply {@code regex} on all ideal graph phases up to and including {@link CompilePhase#BEFORE_MACRO_EXPANSION}.
     */
    private static void macroNodes(String irNodePlaceholder, String regex) {
        IR_NODE_MAPPINGS.put(irNodePlaceholder, new SinglePhaseRangeEntry(CompilePhase.BEFORE_MACRO_EXPANSION, regex,
                                                                          CompilePhase.BEFORE_STRINGOPTS,
                                                                          CompilePhase.BEFORE_MACRO_EXPANSION));
    }


`macroNodes()` could be added after `allocNodes()`, for example.

This allows you to update this IR rule, for example, to:

@IR(failOn = IRNode.MOD_D, phase = CompilePhase.ITER_GVN1)


I suggest to also add a positive rule to check that the IR actually contained the node that you now assume is gone:

@IR(counts = {IRNode.MOD_D, "1"}, phase = CompilePhase.AFTER_PARSING)

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

> 137: 
> 138:     @Test
> 139:     @IR(failOn = {"drem"}, phase = CompilePhase.BEFORE_MATCHING)

This could then be matched on `Compile.PHASEIDEALLOOP1` and the positive rule on `CompilePhase.BEFORE_CLOOPS`, for example.

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

PR Review: https://git.openjdk.org/jdk/pull/23694#pullrequestreview-2649984896
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1974948377
PR Review Comment: https://git.openjdk.org/jdk/pull/23694#discussion_r1974975303