RFR: 8334724: C2: remove PhaseIdealLoop::cast_incr_before_loop()

[Christian Hagedorn]

On Fri, 21 Jun 2024 14:34:09 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.

This looks reasonable to do but could also potentially be a risk. However, I've run t1-7 testing without finding anything and we are early in the release for JDK 24. So, I guess we could take the risk.

src/hotspot/share/opto/loopnode.hpp line 1529:

> 1527:   bool split_thru_phi_could_prevent_vectorization(Node* n, Node* n_blk);
> 1528: 
> 1529: // Check for aggressive application of 'split-if' optimization,

Suggestion:

  // Check for aggressive application of 'split-if' optimization,

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19831#pullrequestreview-2134851262
PR Review Comment: https://git.openjdk.org/jdk/pull/19831#discussion_r1650460208