RFR: 8347018: C2: Insertion of Assertion Predicates ignores the effects of PhaseIdealLoop::clone_up_backedge_goo() [v3]

[Christian Hagedorn]

On Fri, 17 Jan 2025 10:09:51 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java
>>   
>>   Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>
> src/hotspot/share/opto/loopTransform.cpp line 1730:
> 
>> 1728:   const NodeInMainLoopBody node_in_original_loop_body(first_node_index_in_pre_loop_body,
>> 1729:                                                       last_node_index_in_pre_loop_body, old_new);
>> 1730:   create_assertion_predicates_at_main_or_post_loop(pre_loop_head, main_loop_head, node_in_original_loop_body, true);
> 
> Suggestion:
> 
>   const NodeInMainLoopBody node_in_main_loop_body(first_node_index_in_pre_loop_body,
>                                                   last_node_index_in_pre_loop_body, old_new);
>   create_assertion_predicates_at_main_or_post_loop(pre_loop_head, main_loop_head, node_in_main_loop_body, true);
> 
> Would that make sense now?

Definitely! Updated.

> src/hotspot/share/opto/loopTransform.cpp line 1770:
> 
>> 1768: // These have been added by PhaseIdealLoop::clone_up_backedge_goo() and assume to be ending up at the target loop entry
>> 1769: // which is no longer the case when adding additional Assertion Predicates. Fix this by rewiring these nodes to the new
>> 1770: // target loop entry which corresponds to the tail of the last Assertion Predicate before the target loop.
> 
> Could this introduce any circular dependency? I.e. that the Assertion Predicates have dependencies on the control dependencies that we just moved down? -> You could  add a comment here why that is not possible.

I don't think that is possible. I've added an additional comment.

> src/hotspot/share/opto/predicates.hpp line 969:
> 
>> 967:     if (node->_idx < _first_node_index_in_cloned_loop_body) {
>> 968:       Node* cloned_node = _old_new[node->_idx];
>> 969:       return cloned_node != nullptr && cloned_node->_idx >= _first_node_index_in_cloned_loop_body;
> 
> In what case would we return false here? Can you add a comment?

Added!

> src/hotspot/share/opto/predicates.hpp line 1002:
> 
>> 1000:     }
>> 1001:     // Created in PhaseIdealLoop::clone_up_backedge_goo()?
>> 1002:     return node->_idx > _last_node_index_in_pre_loop_body;
> 
> This looks reasonable now, but I'm a little afraid that this could be fragile in the future.
> Hmm, not sure what to do.
> 
> You put a lower bound here. Could we also have an upper bound? Just in case somebody decides to add more nodes in the meantime ... and then you would return true here as well, which would probably be wrong?
> Maybe there could also be some assert, but I'm not sure.

That's a good input. Checking the origin from clones is difficult. But we could do it over storing node indices. I've added some assertion code with checking the last node index used in `clone_up_backedge_goo()`. That feels more robust. Let me know what you think.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1921900549
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1921900829
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1921901007
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1921902515