RFR: 8347018: C2: Insertion of Assertion Predicates ignores the effects of PhaseIdealLoop::clone_up_backedge_goo() [v3]
Emanuel Peter
epeter at openjdk.org
Fri Jan 17 10:36:37 UTC 2025
On Fri, 17 Jan 2025 09:11:12 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:
>> ## Failing Assert
>>
>> The failing assert in `PhaseCFG::schedule_late()` checks the following:
>> https://github.com/openjdk/jdk/blob/55c6904e8f3d02530749bf28f2cc966e8983a984/src/hotspot/share/opto/gcm.cpp#L1431-L1438
>>
>> In the test case, this is violated for `87 storeI`:
>> 
>>
>> The early block `early` for `87 storeI` is bound by `115 loadI` pinned at `161 Region` which is dominated by the control input `146 Region` of `87 storeI`. This lets the assert fail.
>>
>> ## How Did `115 loadI` End up Being Pinned below `87 storeI`?
>> ### Before Pre/Main/Post Loop Creation
>> Before the creation of pre/main/post loops, we have the following graph:
>>
>> 
>>
>> Everything looks fine: The control input of `312 StoreI` (which is eventually cloned and becomes `87 storeI` in the Mach graph) corresponds to the early placement of the store. `415 LoadI` was hoisted out of the loop during Loop Predication and is pinned above at a Template Assertion Predicate.
>>
>> ### Pre/Main/Post Loop Creation
>> #### Post Loop Body Creation
>> During the creation of pre/main/post loops, we clone the main loop body for the post loop body:
>>
>> 
>>
>> We notice that `312 StoreI` is pinned on the main loop backedge. When finishing the last iteration from the main loop and possibly continuing in the post loop, we need to feed everything on the loop backedge of the main loop to the post loop. However, the pinned nodes on the main loop backedge cannot float. Therefore, we need to create new copies of these pinned nodes with `PhaseIdealLoop::clone_up_backedge_goo()`.
>>
>> The pins are updated to the entry of the post loop. All inputs into these pinned nodes that have their current control (fetched with `get_ctrl()`) on the main loop backedge as well are also cloned but keep their control inputs (if any) if it's not the loop backedge.
>>
>> In our example, this applies to `453 StoreI` -> `479 StoreI`, and some inputs recursively (`454 AddI` -> `482 AddI`, `481 LoadI` -> `541 Load`):
>>
>> 
>>
>> Still, all looks fine. Notice that the clone `481 LoadI` of `455 LoadI` is currently still pinned at the same Template Assertion.
>>
>> #### Assertion Predica...
>
> 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>
Looks reasonable :)
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?
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.
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?
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23071#pullrequestreview-2558461123
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1919868506
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1919881434
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1919895109
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1919897832
More information about the hotspot-compiler-dev
mailing list