On Tue, 5 May 2026 16:17:00 GMT, Vladimir Kozlov <[email protected]> wrote:
>> Quan Anh Mai has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Various fixes
>
> Few comments. I did not verify your math - testing should catch any issue.
> Thank you for extending testing.
@vnkozlov @turbanoff Thanks for your suggestions and comments, I have addressed
them.
> src/hotspot/share/opto/divnode.cpp line 99:
>
>> 97: }
>> 98:
>> 99: // magic_divide_constants in divconstants.cpp calculates the constant c,
>> s
>
> "the constant c, s" - what is 's'?
`s` is the shift count. Below we have `floor(x * c / 2**s) + (x < 0 ? 1 : 0)`
> src/hotspot/share/opto/divnode.cpp line 116:
>
>> 114: bool d_pos = divisor >= 0;
>> 115: juint d = g_uabs(divisor);
>> 116: constexpr int N = 32;
>
> Where `N` is used?
I have moved it down near its uses.
> src/hotspot/share/opto/divnode.cpp line 211:
>
>> 209: static Node* transform_int_udivide(PhaseGVN* phase, Node* dividend,
>> juint divisor) {
>> 210: assert(divisor > 1, "invalid constant divisor");
>> 211: constexpr int N = 32;
>
> Can you put `N = 32;` here near it use?
I have moved it down near its uses.
> src/hotspot/share/opto/divnode.cpp line 536:
>
>> 534: if (n->in(0) != nullptr && n->in(0)->is_top()) {
>> 535: return nullptr;
>> 536: }
>
> `remove_dead_region()` should remove this node if its control is top. May be
> use assert here. Or don't check at all.
It only removes the node during IGVN, so during parsing, this is still useful.
> test/hotspot/jtreg/compiler/c2/cr7200264/TestIntVect.java line 519:
>
>> 517:
>> 518: @Test
>> 519: @IR(counts = { IRNode.RSHIFT_VI,
>
> Why this change?
It is because the transformation is better. It does not need to emit the `Add`
for the division anymore.
> test/hotspot/jtreg/compiler/c2/irTests/DivINodeIdealizationTests.java line 88:
>
>> 86: Asserts.assertEQ(a / (13 / 13), identityAgain(a));
>> 87: Asserts.assertEQ(a / -1, divByNegOne(a));
>> 88: Asserts.assertEQ((a & -6) / 2, divByPow2And(a));
>
> It was `(a & -4)` before. Why change? Because it was 0 and you want result
> not 0?
It is because we want something that has the lowest bit being 0. -6 is
0b11...1010 while -4 is 0b11...1100. So -6 is kind of a stricter condition.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/31033#issuecomment-4381356101
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190182969
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190179067
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190177600
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190175594
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190188777
PR Review Comment: https://git.openjdk.org/jdk/pull/31033#discussion_r3190194529
