RFR: 8282365: Consolidate and improve division by constant idealizations [v42]

[Quan Anh Mai]

On Sat, 30 Dec 2023 18:36:19 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   power of 2
>
> test/hotspot/gtest/opto/test_constant_division.cpp line 33:
> 
>> 31: 
>> 32: // Generate a random positive integer of type T in a way that biases
>> 33: // towards smaller values
> 
> Why is there a bias toward smaller numbers?  Maybe it should be named differently to indicate that bias?

Because we are dealing with inputs of division so it makes more sense to have them following somewhat a reciprocal distribution.

> test/hotspot/gtest/opto/test_constant_division.cpp line 54:
> 
>> 52: template <>
>> 53: julong random<jlong, julong>() {
>> 54:   juint bits = juint(os::random()) % 63 + 1;
> 
> This change (`&` => `%`, and the similar change below) go a long way toward explaining why I couldn't
> puzzle out what this function was intended to do.  Note that `&` has lower precedence than `+`, so the
> earlier version was masking with 64.  The new version doesn't have that operator precedence mistake,
> though I'd prefer the precedence be made explicit using parens.

Yes that was my mistake, have added parentheses.

> test/hotspot/gtest/opto/test_constant_division.cpp line 132:
> 
>> 130:   for (int i = 0; i < iter_num;) {
>> 131:     UT d = random<T, UT>();
>> 132:     if ((d & (d - 1)) == 0) {
> 
> We have `is_power_of_2` for this.

This catches `d == 0` also so using `is_power_of_2` is a little misleading I think.

> test/hotspot/gtest/opto/test_constant_division.cpp line 139:
> 
>> 137:     UT N_pos = random<T, UT>();
>> 138:     if (N_neg < d && N_pos < d) {
>> 139:       continue;
> 
> With sufficiently bad luck, we could spin here for a long time.  (Similarly, though much less likely above with
> the power-of-2 case.) That doesn't seem great.  Of course, if one does count these skipped cases against
> the iteration limit then with sufficiently bad luck one might not test anything.  Rather than skipping the test
> here, could you instead modify one of the values and proceed with the test?

Yes I have done that, thanks a lot for your suggestions.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1444028650
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1444028423
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1444028703
PR Review Comment: https://git.openjdk.org/jdk/pull/9947#discussion_r1444028776