RFR: 8347018: C2: assert(find_block_for_node(self->in(0)) == early) failed: The home of a memory writer must also be its earliest placement [v2]
Emanuel Peter
epeter at openjdk.org
Fri Jan 17 06:58:40 UTC 2025
On Tue, 14 Jan 2025 17:56:09 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:
>
> Renaming
You should probably adjust the title of the PR / Bug.
Before I review, I have some understanding questions:
Why is it a problem that the control the store dominates the control of the load? Could such a constellation not happen in other circumstances too?
Could we not come up with some case where the store has its control moved up above the control of the load somehow - or is that generally not supposed to happen?
I suppose I am wondering why can GCM not just handle such cases?
test/hotspot/jtreg/compiler/predicates/assertion/TestLoadPinnedAboveAssertionPredicatesAndUsingStore.java line 30:
> 28: * @summary Test that stores cloned with clone_up_backedge_goo() are not pinned above Assertion Predicates on which a
> 29: * load node is pinned at which will later fail in scheduling.
> 30: * @run main/othervm -Xbatch -XX:CompileCommand=compileonly,*TestLoadPinnedAboveAssertionPredicatesAndUsingStore::test*
Suggestion:
* @run main/othervm -Xbatch -XX:CompileCommand=compileonly,*TestLoadPinnedAboveAssertionPredicatesAndUsingStore::test
Nit 😅
-------------
PR Review: https://git.openjdk.org/jdk/pull/23071#pullrequestreview-2558080582
PR Review Comment: https://git.openjdk.org/jdk/pull/23071#discussion_r1919631609
More information about the hotspot-compiler-dev
mailing list