RFR: 8347365: C2: Fix the handling of depends_only_on_test

[Manuel Hässig]

On Mon, 12 Jan 2026 03:30:35 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

> Hi,
> 
> This PR fixes the handling of `depends_only_on_test` when the control graph is transformed. It has to do with the theoretical idea of `depends_only_on_test`, copy from the JBS issue description:
> 
> To start with, what is `depends_only_on_test`? Given a node `n` with the control input `c`, if `c` can be deduced from `c'` and `n->depends_only_on_test() == true`, then we can rewire the control input of `n` to `c'`. This means that `depends_only_on_test` does not mean that the node depends on a test, it means that the node depends on the test that is its control input.
> 
> For example:
> 
>     if (y != 0) {
>         if (x > 0) {
>             if (y != 0) {
>                 x / y;
>             }
>         }
>     }
> 
> Then `x/y` `depends_only_on_test` because its control input is the test `y != 0`. Then, we can rewire the control input of the division to the outer `y != 0`, resulting in:
> 
>     if (y != 0) {
>         x / y;
>         if (x > 0) {
>         }
>     }
> 
> On the other hand, consider this case:
> 
>     if (x > 0) {
>         if (y != 0) {
>             if (x > 0) {
>                 x / y;
>             }
>         }
>     }
> 
> Then `x/y` does not `depends_only_on_test` because its control input is the test `x > 0` which is unrelated, we can see that if we rewire the division to the outer `x > 0` test, the division floats above the actual test `y != 0`. This means that `depends_only_on_test` is a dynamic property of a node, and not a static property of the division operation. It can change when we transform the graph and it can be different for different nodes of the same kind.
> 
> More details can be found in the description of `Node::depends_only_on_test` and `Node::pin_node_under_control` in this change.
> 
> Please take a look and leave your reviews, thanks a lot.

Thank you for working on this, @merykitty! As far as I can understand it, the fix looks good. I do have a question and a few nitpicks below.

Is this change already tested well enough by the regression tests from JDK-8331717 and JDK-8257822? If so, please add `noreg-sqe`. Or are there possibly additional cases that could be covered?

src/hotspot/share/opto/callnode.hpp line 123:

> 121:   virtual bool  is_CFG() const { return true; }
> 122:   virtual uint hash() const { return NO_HASH; }  // CFG nodes do not hash
> 123:   virtual bool depends_only_on_test() const { return false; }

Why are you not replacing this with an implementation for depends_only_on_test_impl()? Same question for `RethrowNode`, `GotoNode`, and `RegionNode`.

src/hotspot/share/opto/ifnode.cpp line 1577:

> 1575:         igvn->replace_input_of(s, 0, data_target); // Move child to data-target
> 1576:         if (prev_dom_not_imply_this && data_target != top) {
> 1577:           // If prev_dom_not_equivalent, s now depends on multiple tests with prev_dom being the

I think you mean `prev_dom_not_imply_this` here? Also, it would be good to mention that this is to prevent floating above the dependent test.

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

> 1682:   // Mark an IfNode as being dominated by a prior test,
> 1683:   // without actually altering the CFG (and hence IDOM info).
> 1684:   void dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip = false, bool pin_array_access_nodes = false);

Please also rename this `pin_array_access_nodes`, since it is just wired through to `rewire_safe_outputs_to_dominator()`

src/hotspot/share/opto/loopopts.cpp line 1724:

> 1722:         if (!would_sink_below_pre_loop_exit(loop_ctrl, outside_ctrl)) {
> 1723:           if (n->depends_only_on_test()) {
> 1724:             // If this node depends_only_on_test, it will be rewire to a control input that is not the correct test

Suggestion:

            // If this node depends_only_on_test, it will be rewired to a control input that is not the correct test

The same applies to the other changes in this file.

src/hotspot/share/opto/memnode.hpp line 316:

> 314: 
> 315: private:
> 316:   // depends_only_on_test is almost always true, and needs to be almost always

Suggestion:

  // depends_only_on_test_impl is almost always true, and needs to be almost always

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/29158#pullrequestreview-3670265735
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2698496945
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2698448653
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2698029174
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2698367280
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2698305305