RFR: 8336702: C2 compilation fails with "all memory state should have been processed" assert

[Roland Westrelin]

On Mon, 30 Sep 2024 07:02:10 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> When converting a `LongCountedLoop` into a loop nest, c2 needs jvm
>> state to add predicates to the inner loop. For that, it peels an
>> iteration of the loop and uses the state of the safepoint at the end
>> of the loop. That's only legal if there's no side effect between the
>> safepoint and the backedge that goes back into the loop. The assert
>> failure here happens in code that checks that.
>> 
>> That code compares the memory states at the safepoint and at the
>> backedge. If they are the same then there's no side effect. To check
>> consistency, the `MergeMem` at the safepoint is cloned. As the logic
>> iterates over the backedge state, it clears every component of the
>> state it encounters from the `MergeMem`. Once done, the cloned
>> `MergeMem` should be "empty". In the case of this failure, no side
>> effect is found but the cloned `MergeMem` is not empty. That happens
>> because of EA: it adds edges to the `MergeMem` at the safepoint that
>> it doesn't add to the backedge `Phis`.
>> 
>> So it's the verification code that fails and I propose dealing with
>> this by ignoring memory state added by EA in the verification code.
>
> src/hotspot/share/opto/loopnode.cpp line 708:
> 
>> 706:       for (MergeMemStream mms(mem->as_MergeMem()); mms.next_non_empty(); ) {
>> 707:         // Loop invariant memory state won't be reset by no_side_effect_since_safepoint(). Do it here.
>> 708:         // Escape Analysis can add state to mm that it doesn't add to the backedge memory Phis, breaking verification
> 
> Where exactly does that happen in EA?

When an allocation is non escaping and made scalar replaceable, new slices are allocated for the fields of the allocation and the memory graph is updated so allocation/stores/loads to the new slices are connected together. In the process, `MergeMem` nodes need to be updated as well. In this case, I'm not sure this particular `MergeMem` node needs to be updated by EA but it's harmless in any case. The verification code doesn't expect "more" state to be recorded at the safepoint because of the `MergeMem`  than at the backedge.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21009#discussion_r1782460709