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

[Vladimir Kozlov]

On Mon, 24 Jun 2024 12:29:38 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 incrementally with one additional commit since the last revision:
> 
>   review

I am not comfortable with big regression on MacOSX aarch64 even if you can't reproduce it locally. We need to rerun that testing to make sure it is random as you said.

Please, merge latest JDK.

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

PR Comment: https://git.openjdk.org/jdk/pull/19831#issuecomment-2305168673