RFR: 8334724: C2: remove PhaseIdealLoop::cast_incr_before_loop() [v3]
Roland Westrelin
roland at openjdk.org
Mon Aug 26 08:46:41 UTC 2024
> 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
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/19831/files
- new: https://git.openjdk.org/jdk/pull/19831/files/51c093da..bc08cae3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=19831&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=19831&range=01-02
Stats: 86779 lines in 2390 files changed: 49952 ins; 24670 del; 12157 mod
Patch: https://git.openjdk.org/jdk/pull/19831.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19831/head:pull/19831
PR: https://git.openjdk.org/jdk/pull/19831
More information about the hotspot-compiler-dev
mailing list