RFR: 8281429: PhiNode::Value() is too conservative for tripcount of CountedLoop [v7]

[Vladimir Kozlov]

On Mon, 25 Apr 2022 09:29:38 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> The type for the iv phi of a counted loop is computed from the types
>> of the phi on loop entry and the type of the limit from the exit
>> test. Because the exit test is applied to the iv after increment, the
>> type of the iv phi is at least one less than the limit (for a positive
>> stride, one more for a negative stride).
>> 
>> Also, for a stride whose absolute value is not 1 and constant init and
>> limit values, it's possible to compute accurately the iv phi type.
>> 
>> This change caused a few failures and I had to make a few adjustments
>> to loop opts code as well.
>
> Roland Westrelin has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 19 additional commits since the last revision:
> 
>  - undo unneeded change
>  - Merge branch 'master' into JDK-8281429
>  - redo change removed by error
>  - review
>  - Merge branch 'master' into JDK-8281429
>  - undo
>  - test fix
>  - more test
>  - test & fix
>  - other fix
>  - ... and 9 more: https://git.openjdk.java.net/jdk/compare/ea0ce2db...19b38997

There should be correctness tests for MAX_INT,MIN_INT,MAX_LONG,MIN_LONG boundaries, positive and negative strides and `abs(stride) != 1`. All combinations.

src/hotspot/share/opto/cfgnode.cpp line 1120:

> 1118:             if (stride_t->hi_as_long() < 0) {          // Down-counter loop
> 1119:               swap(lo, hi);
> 1120:               jlong first = lo->lo_as_long();

`first` is misleading/confusing name here. I assume `first` is `init` but it is `limit` in this case. I would prefer to have corresponding name to `low limit` of range.

src/hotspot/share/opto/cfgnode.cpp line 1121:

> 1119:               swap(lo, hi);
> 1120:               jlong first = lo->lo_as_long();
> 1121:               if (first < max_signed_integer(l->bt())) {

As I understand this condition is to avoid overflow in next statement. And I thought it should take `stride` into account:

if (first < (max_signed_integer(l->bt()) + stride_t->hi_as_long() + 1)) {

But since we don't know (in general) what final `iv` value would be with `abs(stride) != 1` using value `1` is conservative and correct here.

In short, this condition and following statement needs comment to explain why `1` is used.

src/hotspot/share/opto/cfgnode.cpp line 1123:

> 1121:               if (first < max_signed_integer(l->bt())) {
> 1122:                 first += 1; // lo is after decrement
> 1123:                 // When bounds are constant and ABS(stride) greater than 1, exact bounds for the phi can be computed

The comment is confusing - it sounds like we can calculate it only when `stride` is not 1. I think you mean:

// Exact bounds for the phi can be computed with ABS(stride) greater than 1 when bounds are constant.

src/hotspot/share/opto/cfgnode.cpp line 1124:

> 1122:                 first += 1; // lo is after decrement
> 1123:                 // When bounds are constant and ABS(stride) greater than 1, exact bounds for the phi can be computed
> 1124:                 if (lo->is_con() && hi->is_con() && hi->lo_as_long() > lo->hi_as_long() && stride_t->lo_as_long() != -1) {

`stride` is constant. May be used the value instead of calling `stride_t->lo_as_long()` 3 times in this code.

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

PR: https://git.openjdk.java.net/jdk/pull/7823