RFR: 8334724: C2: remove PhaseIdealLoop::cast_incr_before_loop() [v3]

[Vladimir Kozlov]

On Mon, 26 Aug 2024 08:46:41 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> I propose removing `PhaseIdealLoop::cast_incr_before_loop()` and the
>> `CastII` nodes that it adds at counted loop heads.
>> 
>> They were added to prevent nodes to float above the zero trip guard
>> when the backedge of a counted loop is removed. In particular, when a
>> range check is hoisted by predication, pre/main/post loops are created
>> and if one of the main or post loops lose its backedge, an array load
>> that's control dependent on a predicate above the pre loop could float
>> above the zero trip guard of the main or post loop. That can no longer
>> happen AFAICT with changes related to assert predicates. The array
>> load is now updated to have a control dependency that's below the zero
>> trip guard.
>> 
>> The reason I'm revisiting this is that I noticed that
>> `PhaseIdealLoop::cast_incr_before_loop()` has a bug. When it adds the
>> `CastII`, it looks for the loop phi and picks input 1 of the phi it
>> finds as input to the `CastII`. To find the loop phi, it starts from
>> the loop incremement and loop for a use that's a phi and has the loop
>> head as control. It never checks that the phi it finds is the loop
>> phi. There can be more than one phi as uses of the increment at the
>> loop head and it can pick the wrong one. I tried to write a test case
>> where this would cause a bug but couldn't actually find any use for
>> the `CastII` anymore.
>> 
>> In my testing, the only issue when the `CastII` are not added is that
>> some IR tests for vectorization fails:
>> 
>> compiler/vectorization/TestPopulateIndex.java
>> compiler/vectorization/runner/ArrayShiftOpTest.java
>> compiler/vectorization/runner/LoopArrayIndexComputeTest.java
>> 
>> because removing the `CastII` causes split if to occur with some nodes
>> that take the loop phi as input. That then causes pattern matching
>> during superword to break. I added logic to prevent split if for those
>> cases.
>
> 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 three additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8334724
>  - review
>  - fix

Good.

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

Marked as reviewed by kvn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19831#pullrequestreview-2267389359