RFR: 8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes [v15]

[Emanuel Peter]

On Fri, 29 Nov 2024 11:53:59 GMT, theoweidmannoracle <duke at openjdk.org> wrote:

>> This PR introduces
>> - several new optimizations to unsigned division and modulo
>>    - x % 1, x % x, x % 2^k
>>    - x / 1, x / x, x / 2^k 
>>    - does not implement the Granlund and Montgomery algorithm, which has been implemented for signed modulo division in the past. It is unclear if a lot is to be gained by implementing this.
>> - tests to test existing optimizations for signed division and modulo 
>>    - does not test the Granlund and Montgomery algorithm directly
>
> theoweidmannoracle has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix test

Nice work! I have a few more comments.

src/hotspot/share/opto/divnode.cpp line 461:

> 459: Node* make_and<TypeInt>(Node* a, Node* b) {
> 460:   return new AndINode(a, b);
> 461: }

Does this not belong in `addnode.hpp`? If I searched for something like this, I would look there.

Is there no way you could use `AddNode::make(in1, in2, bt)`, where `bt` is either `T_INT` or `T_LONG`?

src/hotspot/share/opto/divnode.cpp line 474:

> 472: Node* make_urshift<TypeInt>(Node* a, Node* b) {
> 473:   return new URShiftINode(a, b);
> 474: }

There is a similar `LShiftNode::make(Node* in1, Node* in2, BasicType bt)` method... it might be better to follow that code pattern.

src/hotspot/share/opto/divnode.cpp line 477:

> 475: 
> 476: template <typename TypeClass, typename Unsigned>
> 477: Node* unsigned_div_ideal(PhaseGVN* phase, bool can_reshape, Node* div) {

And instead of passing the `TypeClass` here, you could pass the `bt`.

src/hotspot/share/opto/divnode.cpp line 943:

> 941: //------------------------------Idealize---------------------------------------
> 942: Node *UDivINode::Ideal(PhaseGVN *phase, bool can_reshape) {
> 943:   return unsigned_div_ideal<TypeInt, juint>(phase, can_reshape, this);

And here you could pass `bt = T_INT`, instead of the template.

src/hotspot/share/opto/divnode.cpp line 1156:

> 1154:   }
> 1155:   // Don't bother trying to transform a dead node
> 1156:   if (mod->in(0) && mod->in(0)->is_top()) {

Suggestion:

  if (mod->in(0) != nullptr && mod->in(0)->is_top()) {


https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
`Do not use ints or pointers as (implicit) booleans with &&, ||, if, while. Instead, compare explicitly, i.e. if (x != 0) or if (ptr != nullptr), etc.`

src/hotspot/share/opto/type.hpp line 2180:

> 2178: inline const TypeLong* Type::cast<TypeLong>() const {
> 2179:   return is_long();
> 2180: }

I think we usually handle this with `const TypeInteger* isa_integer(BasicType bt)`.

You could also have a look at `jlong get_con_as_long(BasicType bt) const`, may be useful as well in this PR.

test/hotspot/jtreg/compiler/c2/irTests/ModINodeIdealizationTests.java line 43:

> 41: 
> 42:     public static void main(String[] args) {
> 43:         TestFramework.runWithFlags("-XX:CompileCommand=inline,*Math::max");

Is this really necessary? Should this not happen automatically?

test/hotspot/jtreg/compiler/c2/irTests/ModLNodeIdealizationTests.java line 72:

> 70: 
> 71:         Asserts.assertEQ(a % 8589934592L, powerOf2(a));
> 72:         Asserts.assertEQ(a % 8589934591L, powerOf2Minus1(a));

I think we generally prefer to use `1L << 33`, it is easier to read and know what the constant means.

test/hotspot/jtreg/compiler/c2/irTests/UDivLNodeIdealizationTests.java line 120:

> 118:     // Checks x / (c / c) => x
> 119:     public long identityAgainButBig(long x) {
> 120:         return Long.divideUnsigned(x, Long.divideUnsigned(-9214294468834361176L, -9214294468834361176L));  // Long.parseUnsignedLong("9232449604875190440") = -9214294468834361176L

What is the comment for here?

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

PR Review: https://git.openjdk.org/jdk/pull/22061#pullrequestreview-2472212748
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865536747
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865541848
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865542712
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865543843
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865568634
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865550446
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865552790
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865562324
PR Review Comment: https://git.openjdk.org/jdk/pull/22061#discussion_r1865563534