[lworld+vector] RFR: 8314980: [lworld+vector] consider scalarization conditions during ciMultiField creation. [v2]
Jatin Bhateja
jbhateja at openjdk.org
Mon Sep 4 13:30:07 UTC 2023
On Mon, 4 Sep 2023 08:20:25 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.
>>
>>> > An alternative is checking the vector size supported when calculating the `secondary_fields_count()` in `fieldDescriptor.inline.hpp`. Return the multifield count if the vector size is supported, and return `1` if not. And then, we can use this information both at the ci stage when calculating the `nonstatic_fields` in `ciInstanceKlass.cpp` and C2 compiler. WDYT?
>>>
>>
>> I am not in favor of making changes in oop population, unless there is a pressing need, all we need is to furnish appropriate data to compilers through ci layer.
>>
>>> Hi @XiaohongGong , Thanks for reporting this, I was planning to revisit this in next cleanup patch, but its good to club it with this one. Will update the PR with needed modifications.
>>
>> I revisited the implementation and captured the flow in following diagram
>>
>> 
>>
>> As discussed earlier, ClassFileParser creates separate FieldInfo structures for each synthetic multifield as its needed for field layout computations. Interpreter directly operates over oop model structures, currently vectors are created by c2 compile only and it accesses oop model through compiler interface (ci).
>>
>> I think to remove any ambiguity, its best to populate valid bundle_size and set is_multifield_base flag only if target vector size is able to accommodate multifield payload.
>
>> > > > 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.
>
> Your 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.
> > > > > > 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.
-------------
PR Comment: https://git.openjdk.org/valhalla/pull/918#issuecomment-1705273200
More information about the valhalla-dev
mailing list