[lworld+vector] RFR: 8314980: [lworld+vector] consider scalarization conditions during ciMultiField creation. [v2]

[Xiaohong Gong]

On Mon, 4 Sep 2023 13:25:53 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> > > > > > > Hi @jatin-bhateja, thanks for this refactoring! Code is much cleaner to me. So does it also need to clean the special handling for multifields in c1/interpreter (e.g. `deoptimize.cpp`) ? Thanks!
> > > > > > > Besides, the `secondary_fields_count()` which is broadcasted to the `bundle_size()` of the `ciField` is calculated from `multifield_info` (see: https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/runtime/fieldDescriptor.inline.hpp#L81), which I think may not be synced with each other. And it is used widely in C2 and deoptimization. Will it have any issues?
> > > > > 
> > > > > 
> > > > > I do not think it should be a problem for de-optimization, we have separate handling for re-assignment from vector and scalars locations.
> > > > 
> > > > 
> > > > What I'm worried is whether we can remove the `is_multifield` filter and `secondary_fields_count` check in deoptimization. Since I assumed the `secondary_fields_count` is meaningful only when the multifields are vectorized now, otherwise, the normal multifields have been added into the klass's nonstatic_fields, right? If so, only check whether the field is vector is enough for me.
> > > 
> > > 
> > > You assumption still holds good, secondary_fields_count is usable only if multifiled is held in a vector, but we need secondary_fields_count to estimate the vector length unless VectorSupport::klass2length is re-introduced. I agree we can do some more cleanup here.
> > 
> > 
> > Yes, I agree it is needed. I mean the filter for multifield (see: https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/runtime/deoptimization.cpp#L1567) and the scalar field reassign loop with `secondary_fields_count` as the loop limit. Can these two part be cleaned like before?
> 
> Hi @XiaohongGong , I am not in favor of modifying oops structures which are populated during raw bytecode parsing, while we can always prune ci model to present ciField/ciMultifield structures in desired format based on target supported vector size, but a change in oop model will also trigger changes in field layout computations.
> 
> An iteration over raw fields of a klass should see a separate FieldInfo for each synthetic and base multifield. Also Oops model is common across different execution engines (Compilers and Interpreter), while C2 has ability to create vector IR other do not. Hence by construction Oops model should not be influenced by target vector size.
> 
> We can revisit this at a later stage if needed.

OK, make sense to me. I assumed that you'v verified the latest change with vector api jtreg tests, and no new regression is found, right?

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

PR Comment: https://git.openjdk.org/valhalla/pull/918#issuecomment-1705844608