[lworld+vector] RFR: 8314628: [lworld+vector] validation regression fixes and cleanups. [v2]

[Xiaohong Gong]

On Wed, 23 Aug 2023 03:00:35 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> src/hotspot/share/ci/ciInstanceKlass.cpp line 572:
>> 
>>> 570: 
>>> 571:   return mfield;
>>> 572: }
>> 
>> This whole logic is quite similar with the one in `compute_nonstatic_fields_impl` when the field is a multifield. But the code style is not the same. For example, we check the `sec_fields_count` when field is multifield. This is right to me, in case other multifield is appended to a previous multifield_base which is not its expected one, although we do not have such cases now. But this similar check is missing in `compute_nonstatic_fields_impl`, isn't it? 
>> 
>> Another suggestion is: can we directly use `ciMultiField` instead of `ciField` as the method argument type? Besides, may I ask why we have to new another `ciMultiField`, set the secondary_fields to it and return? Can we modify the input field instead and change the method's return type to `void`?
>
> Yes, _compute_nonstatic_fields_impl_ is only called from [ InlineTypeNode constructor](https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/opto/inlinetypenode.hpp#L40), but while parsing field access bytecodes "withfield/getfield/putfield" both c1 and c2 compiler uses _ciBytecodeStream::get_field() API_,  since it directly operates over bytecode hence does not take into account the effect of @multifield annotation over it and earlier used to create only one ciField for base multifield instead of creating a hierarchical ciMutifield structure.
> 
> In addition, ClassFiledParser creates[ contiguous sequence of FieldInfo ](https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/classfile/classFileParser.cpp#L1623) for both base and subsequent synthetic multifield, hense it's not possible to associate synthetic multifields with any other base multifield.
> 
>>>> But this similar check is missing in compute_nonstatic_fields_impl, isn't it?
> 
> We can add an assertion to check that secondary fields count always match the number of synthetic FieldInfos after base multifield.

Sounds good to me. Thanks!

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/909#discussion_r1302424648