RFR: 8282365: Optimize divideUnsigned and remainderUnsigned for constants [v16]

[Emanuel Peter]

On Mon, 3 Jul 2023 12:19:43 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 50 commits:
>> 
>>  - missing java_negate
>>  - Merge branch 'master' into unsignedDiv
>>  - whitespace
>>  - move asserts to use sites
>>  - windows complaints
>>  - compiler complaints
>>  - undefined internal linkage
>>  - add tests, special casing large shift
>>  - draft
>>  - Merge branch 'master' into unsignedDiv
>>  - ... and 40 more: https://git.openjdk.org/jdk/compare/5b147eb5...eb1f5dd9
>
> src/hotspot/share/opto/divnode.cpp line 441:
> 
>> 439:     jlong magic_const;
>> 440:     jint shift_const;
>> 441:     bool magic_const_ovf;
> 
> `does_magic_const_overflow` Would that work too?

I'm not sure exactly what this boolean means, and it is making it diffucult to undersand the logic below

> src/hotspot/share/opto/divnode.cpp line 947:
> 
>> 945: 
>> 946:   // Divisor very large, constant 2**31 can be transform to a shift
>> 947:   if (ti->_hi <= 0 && ti->_hi > min_jint) {
> 
> Would be easier to read as a range like this
> Suggestion:
> 
>   if (min_jint < ti->_hi && ti->_hi <= 0) {

Do we have test cases for this?

> src/hotspot/share/opto/divnode.cpp line 956:
> 
>> 954:     return nullptr;
>> 955:   }
>> 956:   juint i = ti->get_con();       // Get divisor
> 
> I'd replace `i` with `divisor_con`.

Or at the very least `ti_con`.

> src/hotspot/share/utilities/javaArithmetic.cpp line 71:
> 
>> 69: 
>> 70:   assert(M < java_shift_left(jlong(1), 32), "");
>> 71:   assert(s < 32, "");
> 
> Just in case: assert that they are non-negative (`>=0`)

Generally, take the asserts into all of these methods, so that they are also tested in the gtest

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

PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1250806030
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1250834360
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1250838627
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1250875841