RFR: 8328528: C2 should optimize long-typed parallel iv in an int counted loop [v9]

[Emanuel Peter]

On Mon, 26 Aug 2024 23:25:47 GMT, Kangcheng Xu <kxu at openjdk.org> wrote:

>> Currently, parallel iv optimization only happens in an int counted loop with int-typed parallel iv's. This PR adds support for long-typed iv to be optimized. 
>> 
>> Additionally, this ticket contributes to the resolution of [JDK-8275913](https://bugs.openjdk.org/browse/JDK-8275913). Meanwhile, I'm working on adding support for parallel IV replacement for long counted loops which will depend on this PR.
>
> Kangcheng Xu 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 27 additional commits since the last revision:
> 
>  - Merge branch 'master' into long-typed-parallel-iv
>  - use @run driver and Argument.RANDOM_ONCE
>  - Merge branch 'master' into long-typed-parallel-iv
>  - add random strides to tests
>  - fix tests on larger strides
>  - add more expressive comments and test cases
>  - Merge branch 'master' into long-typed-parallel-iv
>  - update comments to clarify on type casting
>  - add pseudocode for subgraphs before/after the transformation
>  - remove WIP support for long counted loops
>  - ... and 17 more: https://git.openjdk.org/jdk/compare/e94d582e...20bdc791

Sorry for the delay, I was out of the office and am going on vacation again soon - lots to do not enough time.

Things look much better now! A few more comments.

For the optmization to work, we need constant `stride_con` and `stride_con2`.
But `init2`, `init` and `limit` are variables. I am missing tests where they are actually variables, I think you at most make `init` and `limit` variables, but only individually. And you always have `init2 = 0`. I think it would be quite important to have some variable cases as well.

As I said: I'll be away soon, so feel free to ping other reviewers if I don't respond ;)

src/hotspot/share/opto/loopnode.cpp line 4000:

> 3998: 
> 3999:     // The ratio of the two strides cannot be represented as an int
> 4000:     // if stride_con2 is min_int and stride_con is -1.

You could adjust the comment to mention both `min_jint and min_jlong`.

src/hotspot/share/opto/loopnode.cpp line 4052:

> 4050: 
> 4051:       Node* ratio_idx = MulNode::make(phi_converted, ratio, stride_con2_bt);
> 4052:       _igvn.register_new_node_with_optimizer(ratio_idx, phi_converted);

This block has some code duplication. Could you refactor it somehow to make it more concise?

test/hotspot/jtreg/compiler/loopopts/parallel_iv/TestParallelIvInIntCountedLoop.java line 31:

> 29: import compiler.lib.ir_framework.IRNode;
> 30: import compiler.lib.ir_framework.Test;
> 31: import compiler.lib.ir_framework.TestFramework;

Suggestion:

import compiler.lib.ir_framework.*;

I think that should suffice. But up to you.

test/hotspot/jtreg/compiler/loopopts/parallel_iv/TestParallelIvInIntCountedLoop.java line 51:

> 49:         stride = new Random().nextInt(1, Integer.MAX_VALUE / 16);
> 50:         stride2 = stride * new Random().nextInt(1, 16);
> 51:     }

So `stride` and `stride2` are compile-time constants. That is intended, right?

test/hotspot/jtreg/compiler/loopopts/parallel_iv/TestParallelIvInIntCountedLoop.java line 75:

> 73: 
> 74:         return a;
> 75:     }

You have `failOn` for every test. I'm worried that something with the test could be so wrong that we just don't have a `LoopNode` for some completely wrong reason.
Can you please add a "control test", that actually has a simple loop, and where you can find it with an `@IR` check?

test/hotspot/jtreg/compiler/loopopts/parallel_iv/TestParallelIvInIntCountedLoop.java line 255:

> 253: 
> 254:         return a;
> 255:     }

I'm also missing some cases where both the `init` and `limit` are variable.
You could also sprinkle in a case or two where we use `<=` instead of `<`.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18489#pullrequestreview-2313484847
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765532812
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765538519
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765539228
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765545340
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765542098
PR Review Comment: https://git.openjdk.org/jdk/pull/18489#discussion_r1765544249