[lworld] RFR: 8369045: [lworld] valhalla/valuetypes/WeakReferenceTest.java has an unschedulable graph

[Damon Fenacci]

On Thu, 11 Dec 2025 16:01:00 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> # Issue
>> A few test failed intermittently with -Xcomp on Mac due to an unschedulable graph in C2:
>> - valhalla/valuetypes/WeakReferenceTest.java
>> - valhalla/valuetypes/ProxyTest.java
>> - valhalla/valuetypes/ObjectNewInstance.java
>> - valhalla/valuetypes/ObjectMethodsViaCondy.java
>> 
>> # Causes
>> 
>> The origin of the issue seems to be coming from strength reduction (`process_late_inline_calls_no_inline`) where we replace virtual and MH calls with direct calls. 
>> https://github.com/dafedafe/valhalla/blob/75e2dd95df5d847d7d6e35a23016d22705681cf4/src/hotspot/share/opto/compile.cpp#L3072
>> If the return type of the methods are not loaded, we add a call to runtime's `store_inline_type_fields_to_buf` right after the actual method call, to save the scalarized return into a oop. This happens first for the caller at parse time and then for the callee when strength-reducing the virtual call to a direct one. The return projections of the inline fields of the call are added to `store_inline_type_fields_to_buf` and its oop projection is then added as input to the other `store_inline_type_fields_to_buf` which fundamentally leaves the graph around it in an awkward state.
>> 
>> If this happens in combination with loop unswitching it can lead to a graph not being schedulable, which is what happens in the failure of this issue, where we have:
>> * a virtual call with a following `store_inline_type_fields_to_buf` (1) in a loop.
>> * the loop undergoes unswitching, which creates 2 copies of the body (including a copy of the virtual call). All outputs of the new virtual call are phi-d with the one of the other path as input of the `store_inline_type_fields_to_buf`.
>> * the new copy of the virtual call is later replaced with a direct call: the creation of the new direct call adds a `store_inline_type_fields_to_buf` (2) right after the direct call. All the inline type return values of the call being inlined are removed, so the phis now only have one input and are removed by a later GVN pass.
>> <img width="600" alt="Screenshot 2025-11-26 at 10 33 51" src="https://github.com/user-attachments/assets/35e947c2-350e-414f-9d44-72559d480a88" />
>> 
>> * this creates an issue later during GCM since the `store_inline_type_fields_to_buf` (1) call is logically not dominated by any of the 2 sides of the unswitched loop and lands in a separate dominance path of the arguments whose phis have been removed (which are still dominated by the original virtual call).
>> 
>> # Solution
>> 
>> The issue ha...
>
> src/hotspot/share/opto/graphKit.cpp line 2052:
> 
>> 2050: 
>> 2051:         // Don't add store to buffer call if we are strength reducing
>> 2052:         if (!C->strength_reduction()) {
> 
> Can we use `_gvn.is_IterGVN() && !C->inlining_incrementally()` here instead? Assuming that whenever we call this after parsing and when not incrementally inlining, we are doing post-parse devirtualization of a call.

That's a possibility. I've also considered checking for `!C->inlining_incrementally() && C->late_inline_count() > 0` but I'm a bit torn between being 100% sure we are doing strength reduction but using a compile "flags" (a bit 🫤) and checking for other, already available, conditions (like the one you suggest), which make me a bit unsure and could potentially change in the future...
But I guess introducing a new field to `Compile` just for this should be avoided. So, let's go for `_gvn.is_IterGVN() && !C->inlining_incrementally()` 🙂

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1768#discussion_r2614778037