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

[Quan Anh Mai]

On Thu, 22 Jan 2026 15:04:52 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Quan Anh Mai has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - more clarification
>>  - Refine comments
>
> 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.

This method tries to remove `this` and rewires all nodes that `depends_only_on_test` to `prev_dom`. `prev_dom_not_imply_this` means that whether the test at `this` can be implied from the test at `prev_dom` only. For example:

    if (x > 0) {
        if (y != 0) {
            if (x != -1) {
                r = y / (x + 1);
            }
        }
    }

`x != -1` can be implied solely from `x > 0`. As a result, when moving the division to the `IfProj` of the test `x > 0`, the division still `depends_only_on_test`.

On the other hand,

    if (x != y) {
        if (y == -1) {
            if (x != -1) {
                r = y / (x + 1);
            }
        }
    }

We can remove the test `x != -1`, since it can be implied from `x != y` and `y == -1`. But now `x != -1` cannot be implied solely from `y == -1`. As a result, the division must cease to `depends_only_on_test`. This is the meaning of the parameter name `prev_dom_not_imply_this`.

> 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?

Any node that `depends_only_on_test` must be pinned because it does not `depends_only_on_test` anymore. If we somehow hoist that node afterwards because we find an equivalent test, then it is an incorrect optimization. This is the cause of the previous issues with `DivNode`s, they fail to be pinned when they cannot `depends_only_on_test` anymore.

> 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.

Similar to above, all nodes that `depends_only_on_test` must be pinned.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717464707
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717471714
PR Review Comment: https://git.openjdk.org/jdk/pull/29158#discussion_r2717473056