[lworld] RFR: 8278390: [lworld] Scalarization of nullable inline types in the calling convention [v2]

[Frederic Parain]

On Thu, 21 Apr 2022 14:06:52 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> This is a large and complex change that extends the calling convention optimization to support nullable types. B2 (value classes) and B3.ref (nullable primitive classes) in arguments and return values are now scalarized, i.e., passed and returned as fields instead of as references. They should now behave like B3.val with the overhead of null-handling.
>> 
>> **Major changes:**
>> - Use information from the preload attribute to set up the scalarized calling convention at method link time and support it in the interpreter, adapters, stubs, C1 and C2 on x86_64 and aarch64.
>> - An additional `IsInit` field is passed for each argument to keep track of null. This field will later be re-used to also pass the buffer oop if the value is non-null and buffered (see Limitations section below).
>> - No additional field is required for returns. Instead, the first register is re-used to hold the `IsInit` information. It now contains either (i) all zero if the value is null, (ii) an oop if the value is non-null and heap buffered or (iii) a tagged klass pointer if the value is non-null and not heap buffered. Since the GC wouldn't be happy to observe a value that can be an oop or a tagged klass pointer, we need to clean it up right after the call. We do this by setting it to zero if it's tagged and extracting the IsInit information into another register. Both can then be used by compiled code. If all registers are occupied by the return values, we put the IsInit information into a reserve stack slot (see logic in the `enc_class call_epilog` in the `.ad` files).
>> - Many bug fixes (also unrelated ones), new correctness and IR verification tests.
>> - The long term goal is to get rid of the `InlineTypeNode` / `InlineTypePtrNode` duality in C2. This change makes another step in that direction but there is significant work left.
>> 
>> **Limitations:**
>> - Similar to scalarization of B3.val in arguments, the buffer oop is currently not passed as "souvenir". This is non-trivial because it requires some effort to ensure that buffer allocations are not kept alive when "escaping" through arguments at calls and will be addressed by a future change.
>> - Preload attribute mismatches are not yet handled.
>> - Above issues and all the `// TODO 8284443` comments, will be addressed by the follow-up enhancement [JDK-8284443](https://bugs.openjdk.java.net/browse/JDK-8284443).
>> 
>> Thanks,
>> Tobias
>
> Tobias Hartmann has updated the pull request incrementally with one additional commit since the last revision:
> 
>   equal -> zero cleanup

Runtime, CI and C1 changes look good to me.

Fred

src/hotspot/cpu/x86/interp_masm_x86.cpp line 1179:

> 1177: 
> 1178:   if (state == atos && InlineTypeReturnedAsFields) {
> 1179:     // Check if we are returning an inline type and load its fields into registers

Suggestion:
// Check if we are returning an non-null inline type and load its fields into registers

src/hotspot/share/runtime/sharedRuntime.cpp line 2975:

> 2973:         InlineKlass* vk = ss.as_inline_klass(holder);
> 2974:         // TODO 8284443 Mismatch handling, we need to check parent method args (look at klassVtable::needs_new_vtable_entry)
> 2975:         if (vk != NULL && (bt == T_PRIMITIVE_OBJECT || holder->is_preload_class(vk->name())) &&

Limiting argument scalarization to value classes present in the PreLoad attribute could lead to missed opportunities of scalarize if the class was loaded but not in the attribute (incomplete or missing PreLoad attributer case). The calling convention could instead look at which classes are loaded with no consideration to what triggered the loading. This would force overriding methods to look directly at the calling convention of the overridden method instead of looking at its context (lines 2861, 2882).
But this is a work that should be done when dealing with calling conventions mismatches (JDK-8284443).

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

Marked as reviewed by fparain (Committer).

PR: https://git.openjdk.java.net/valhalla/pull/685