RFR: 8332442: C2: refactor Mod cases in Compile::final_graph_reshaping_main_switch() [v5]

[Christian Hagedorn]

On Tue, 10 Sep 2024 21:58:41 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> Hello all. This patch addresses [JDK-8332442](https://bugs.openjdk.org/browse/JDK-8332442) and refactors `Op_ModI`/`Op_ModL`/`Op_UModI`/`Op_UModL` cases in `DIVMOD` transforamtions. The purpose of the transformation to convert adjacent div `/`  and mod `%` operations of the same operands into one should the platform support this feature (e.g., x86-64).
>> 
>> I took the liberty adding _signed_ DIVMOD nodes (i.e., `DIV_MOD_I` and `DIV_MOD_L`) to the `IRNode` class constants as they are previously missing. Please let me know if they were left intentionally and if there are any other concerns. Thanks!
>> 
>> ~This will be a draft PR before GHA tests are confirmed passing.~
>
> Kangcheng Xu has updated the pull request incrementally with one additional commit since the last revision:
> 
>   allow one div on platforms without hardware divmod

Generally the clean-up idea is good! A few comments.

src/hotspot/share/opto/compile.cpp line 3169:

> 3167:   }
> 3168: 
> 3169:   // Check if a%b and a/b both exist

I suggest to add whitespaces for better readability (same below):
Suggestion:

  // Check if "a % b" and "a / b" both exist

src/hotspot/share/opto/compile.cpp line 3171:

> 3169:   // Check if a%b and a/b both exist
> 3170:   Node* d = n->find_similar(Op_DivIL(bt, is_unsigned));
> 3171:   if (!d) {

You should directly compare against `nullptr`:
Suggestion:

  if (d == nullptr) {

src/hotspot/share/opto/compile.cpp line 3181:

> 3179:     n->subsume_by(divmod->mod_proj(), this);
> 3180:   } else {
> 3181:     // replace a%b with a-((a/b)*b)

Suggestion:

    // Replace "a % b" with "a - ((a / b) * b)"

test/hotspot/jtreg/compiler/c2/TestDivModNodes.java line 33:

> 31: /*
> 32:  * @test
> 33:  * @summary Test DIV and MOD nodes are converted into DIVMOD where possible

Suggestion:

 * @summary Test that DIV and MOD nodes are converted into DIVMOD where possible

test/hotspot/jtreg/compiler/c2/TestDivModNodes.java line 36:

> 34:  * @library /test/lib /
> 35:  * @run main/othervm -XX:+UseDivMod compiler.c2.TestDivModNodes
> 36:  * @run main/othervm -XX:-UseDivMod compiler.c2.TestDivModNodes

You should not pass the flags like that to the IR framework tests. It will not take these flags into account. What you should do instead is using the IR framework provided methods:


 * @run driver compiler.c2.TestDivModNodes
...
    public static void main(String[] args) {
        TestFramework.runWithFlags("-XX:-UseDivMod");
        TestFramework.runWithFlags("-XX:+UseDivMod");
    }

Note that `UseDivMod` is a C2 specific flag. So, you should also add a `@requires vm.compiler2.enabled`.

test/hotspot/jtreg/compiler/c2/TestDivModNodes.java line 58:

> 56: 
> 57:         verifyResult(dividend, divisor, q, r, TestDivModNodes::signedIntDiv, TestDivModNodes::signedIntMod);
> 58:     }

Couldn't you just make `q` and `r` fields and then check them from within a `@Run` method?

Something like that (not tested):

static final Random RANDOM = AbstractInfo.getRandom();
int divResult;
int modResult;
...
@Test
@IR(...)
void testSignedIntDivMod(int dividend, int divisor) {
    divResult = dividend / divisor;
    modResult = dividend % divisor;
}

@Run(test="testSignedIntDivMod")
void run() {
    int q = RANDOM.nextInt();
    int r = RANDOM.nextInt();
    testSignedIntDivMod(q, r);
    Asserts.assertEQ(q / r, divResult);
    Asserts.assertEQ(q % r, modResult);
}

Since the IR framework is single threaded, it should not be a problem to pass the result like that over fields.

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

Changes requested by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20877#pullrequestreview-2296498167
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754117764
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754124805
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754118969
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754132715
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754157503
PR Review Comment: https://git.openjdk.org/jdk/pull/20877#discussion_r1754248381