RFR: 8347365: C2: Fix the handling of depends_only_on_test [v2]

[Roland Westrelin]

On Fri, 16 Jan 2026 16:04:49 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.
>
> Quan Anh Mai has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - more clarification
>  - Refine comments

As I understand, this change removes logic that's overly conservative but doesn't address any correctness issue (i.e. there's no crash or incorrect execution that this fixes). Given the new logic is less conservative, there should be cases where the code optimizes better with this change. Would it make sense to add IR test cases to catch regressions?

src/hotspot/share/opto/cfgnode.hpp line 463:

> 461:   static Node* up_one_dom(Node* curr, bool linear_only = false);
> 462:   bool is_zero_trip_guard() const;
> 463:   Node* dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool prev_dom_not_imply_this);

I find `prev_dom_not_imply_this` confusing. I'm actually not sure I understand why you named it that way.

src/hotspot/share/opto/memnode.cpp line 1030:

> 1028: LoadNode* LoadNode::pin_array_access_node() const {
> 1029:   const TypePtr* adr_type = this->adr_type();
> 1030:   if (adr_type != nullptr && adr_type->isa_aryptr()) {

As I understand you got rid of the check for an array access. Are there non array loads that we would like pinned? Are there loads that are control dependent on a condition and are non array loads that now gets pinned when they don't need to?

src/hotspot/share/opto/split_if.cpp line 719:

> 717: 
> 718:   _igvn.remove_dead_node(region);
> 719:   if (iff->Opcode() == Op_RangeCheck) {

Do we want really want to pin array accesses when a condition other that a `RangeCheck` is eliminated? It seems overly conservative to me. The risk is we would pin nodes that don't need to.

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

PR Review: https://git.openjdk.org/jdk/pull/29158#pullrequestreview-3692952131
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717306115
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717354428
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717373046