[lworld] RFR: 8331006: [lworld] Support of null markers for nullable flat fields [v2]

[Frederic Parain]

On Fri, 26 Apr 2024 17:21:16 GMT, Dan Heidinga <heidinga at openjdk.org> wrote:

>> Frederic Parain has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixing issues spotted during reviews
>
> src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3355:
> 
>> 3353:   // access field
>> 3354:   switch (bytecode()) {
>> 3355:   case Bytecodes::_fast_vputfield: //fall through
> 
> Suggestion:
> 
>   case Bytecodes::_fast_vputfield:

Fixed in next commit.

> src/hotspot/share/classfile/fieldLayoutBuilder.cpp line 800:
> 
>> 798:       bool value_has_oops = field_is_known_value_class ? _inline_type_field_klasses->at(fieldinfo.index())->nonstatic_oop_count() > 0 : true;
>> 799:       bool is_candidate_for_flattening = fieldinfo.field_flags().is_null_free_inline_type() || (EnableNullableFieldFlattening && field_is_known_value_class && !value_has_oops);
>> 800:       // if (!fieldinfo.field_flags().is_null_free_inline_type()) {
> 
> Is it worth pulling this into a `array_candidate_for_flattening` helper method?  It's a complex set of conditions and this is the second occurrence of it...

I don't like the duplication either, and having a helper method would make the code easier to read. However, this is a section of code under big transformations, with nullable flat field being added, then next the atomic fields, then the nullable atomic fields. Before refactoring, I'd prefer to wait until all the cases are covered, so we know exactly all the parameters the helper method will require, and if the two cases are still identical or if some divergences appeared.

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1586329253
PR Review Comment: https://git.openjdk.org/valhalla/pull/1078#discussion_r1586328862