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

[Christian Hagedorn]

On Mon, 20 Jan 2025 07:23:30 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`:
>> ![image](https://github.com/user-attachments/assets/6844e060-69c4-436e-ac47-c14ec59ee4d2)
>> 
>> 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:
>> 
>> ![image](https://github.com/user-attachments/assets/3f6e6b92-5194-4efd-89a9-246a977e3022)
>> 
>> 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:
>> 
>> ![image](https://github.com/user-attachments/assets/e8de3d6d-34c3-46fd-abd4-df212e2e77fb)
>> 
>> 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`):
>> 
>> ![image](https://github.com/user-attachments/assets/0d4b818b-5c23-4ad9-b474-976c352f4c88)
>> 
>> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Review Emanuel
>  - Merge branch 'master' into JDK-8347018
>  - Update test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java
>    
>    Co-authored-by: Emanuel Peter <emanuel.peter at oracle.com>
>  - Renaming
>  - New NodeInMainLoopBody
>  - 8347018: C2: assert(find_block_for_node(self->in(0)) == early) failed: The home of a memory writer must also be its earliest placement

src/hotspot/share/opto/loopTransform.cpp line 1455:

> 1453:   // dependencies.
> 1454: 
> 1455:   assert(post_head->in(1)->is_IfProj(), "must be zero-trip guard If node projection of the post loop");

I somehow moved this down after testing - I cannot remember why. It should be further up. Fixed here as well.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1921904404