RFR: 8323154: C2: assert(cmp != nullptr && cmp->Opcode() == Op_Cmp(bt)) failed: no exit test

[Tobias Hartmann]

On Wed, 17 Jan 2024 07:44:28 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> This bug is very similar to [JDK-8314191](https://bugs.openjdk.org/browse/JDK-8314191) but with long counted loops instead of int counted loops and a with a different manifestation. 
> 
> The original problem was that [JDK-8276162](https://bugs.openjdk.org/browse/JDK-8276162) added transformations for `CmpI` nodes to use `CmpU` nodes instead. The transformations were also applied for `CmpI` nodes of counted loop exit checks which messed pattern matching up. [JDK-8314191](https://bugs.openjdk.org/browse/JDK-8314191) and the follow-up fix [JDK-8316719](https://bugs.openjdk.org/browse/JDK-8316719) fixed this but only for `CountedLoopNodeEndNodes`. The newly added `is_cloop_condition()` method only checks for `is_CountedLoopEnd()` (int counted loops) instead of `is_BaseCountedLoopEnd()` (also includes long counted loop). This patch fixes this.
> 
> I've had a closer look at other uses of `is_CountedLoop*` and found that
> https://github.com/openjdk/jdk/blob/1007618f6f97fad0f66e4074b50521bdd853629e/src/hotspot/share/opto/subnode.cpp#L126-L139
> and
> https://github.com/openjdk/jdk/blob/1007618f6f97fad0f66e4074b50521bdd853629e/src/hotspot/share/opto/subnode.cpp#L155-L163
> should probably also use the `BaseCountedLoop*` versions. These methods are used in `ok_to_convert()` which is used in several places. They try to prevent transformations involving the iv and the increment node of a counted loop to save registers. However, these transformations are still applied before a loop is transformed to a counted loop. This raises the question whether these bailouts should be extended to also work before loop opts. Since this code has been around for such a long time, it would also be interesting to see, if it's still beneficial to block these optimizations in general. If so, it might be good if we could add some IR tests to prove that.
> 
> There are more places where we try to prevent such transformations but again only if we already have a counted loop. For example:
> https://github.com/openjdk/jdk/blob/1007618f6f97fad0f66e4074b50521bdd853629e/src/hotspot/share/opto/addnode.cpp#L176-L191
> or
> https://github.com/openjdk/jdk/blob/1007618f6f97fad0f66e4074b50521bdd853629e/src/hotspot/share/opto/addnode.cpp#L193-L209
> 
> It might be a good idea to revisit all of these bailouts in general and check if it's still beneficial to have them around and if they should be extended to also work before loop opts. 
> 
> I suggest to do this investigation together with fixing `CountedLoop*` -> `BaseCountedLoop*` in...

Looks good to me too.

> I suggest to do this investigation together with fixing CountedLoop* -> BaseCountedLoop* in the methods used in ok_to_convert() in a separate RFE.

Makes sense.

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

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17459#pullrequestreview-1826774130