[jdk17u-dev] RFR: 8303466: C2: failed: malformed control flow. Limit type made precise with MaxL/MinL

[Roland Westrelin]

On Tue, 25 Jun 2024 04:46:52 GMT, Martin Balao <mbalao at openjdk.org> wrote:

> Hi,
> 
> I would like to propose a backport of 8303466 [1] to jdk17u. jdk17u can benefit from this fix and having more accurate limit type information.
> 
> The JDK main line patch (introduced for JDK 21) does not apply cleanly because of the following:
> 
>  * src/hotspot/share/opto/convertnode.cpp
>    * jdk17u does not have 8230382 so the context where the change applies is different. Manually applied the change.
> 
>  * src/hotspot/share/opto/loopTransform.cpp
>    * jdk17u does not have 8294540 that removes the protection of the limit under an Opaque2Node. This is orthogonal to the change introduced by 8303466, so there is no need to make 8294540 a dependency.
>    * jdk17u does not have 8299975 that assigns more accurate type information to the CMoveINode node. This is not necessary with 8303466 because the MaxLNode or MinLNode macro nodes (inserted instead of the CMoveINode node during PhaseIdealLoop) will have accurate type information, even more accurate than CMoveINode which used the original limit for one of the range ends (intead of the limit minus the strides after unrolling). See MaxLNode::add_ring and MinLNode::add_ring.
>    * Note: even with 8299975 backported, 8303466 wouldn't apply cleanly because 8301074 was backported already and it applies changes to code introduced by 8299975 (these changes were not in 17u's 8301074 and would have to be applied manually).
>   * The CastIINode node inserted during the first unrolling to keep precise type information after inserting an Opaque2 node is still needed, but has to be inserted before the node is used for ConvI2LNode -> SubLNode -> MaxLNode/MinLNode -> ... Otherwise, bottom type propagates accross this nodes path. The type of the node has to reflect the original limit type. MaxLNode/MinLNode will handle the clamping effect over the type.
> 
> In addition to the previous conflicts, the following changes were applied:
> 
>  * test/hotspot/jtreg/compiler/loopopts/TestLoopLimitSubtractionsCollapse.java
>  * test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java
>    * jdk17u does not have 8280378 (which, in turn, depends on 8280076 and is not in jdk17u either). These are large enhancements that extend the IR framework for tests and backporting them would be a large effort. The test is making an assertion in an intermediate graph to check the number of MaxL/MinL nodes, and make sure that the optimization that merges them (MaxLNode::Ideal/MinLNode::Ideal) was applied. These macro nodes are not longer available after the Macro expansi...

That looks good to me.

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

Marked as reviewed by roland (Reviewer).

PR Review: https://git.openjdk.org/jdk17u-dev/pull/2635#pullrequestreview-2137710772