On Mon, 15 Dec 2025 09:36:55 GMT, Damon Fenacci <[email protected]> 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... > > Damon Fenacci has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8369045: remove strength reduction flag src/hotspot/share/opto/graphKit.cpp line 2051: > 2049: _gvn.set_type(ret, ret->Value(&_gvn)); > 2050: > 2051: // Don't add store to buffer call if we are strength reducing Why do we need to add the above runtime check of the result again when doing strength reduction? src/hotspot/share/opto/graphKit.cpp line 2226: > 2224: ((_gvn.is_IterGVN() && !C->inlining_incrementally()) && > InlineTypeReturnedAsFields && > !call->as_CallJava()->method()->return_type()->is_loaded()), > 2225: "unexpected number of results"); > 2226: // If we are doing strength reduction and the return types is not > loaded we Suggestion: // If we are doing strength reduction and the return type is not loaded we ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1768#discussion_r2622361763 PR Review Comment: https://git.openjdk.org/valhalla/pull/1768#discussion_r2622358572
