8214307 [lworld][C1] Access to flattened fields does not support nested flattened fields

Ioi Lam ioi.lam at oracle.com
Tue Nov 27 21:40:03 UTC 2018


Looks good!

Thanks

- Ioi

On 11/27/18 12:55 PM, Frederic Parain wrote:
> Thank you for spotting the useless recursion.
> I’ve updated the patch to simplify the copy_value_content() method:
>
> http://cr.openjdk.java.net/~fparain/C1/webrev.03/index.html
>
> Fred
>
>
>> On Nov 27, 2018, at 12:44, Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>> Then I don't understand this
>>
>>
>> On 11/27/18 1:23 AM, Tobias Hartmann wrote:
>>> Hi Ioi,
>>>
>>> On 27.11.18 08:40, Ioi Lam wrote:
>>>> BTW, looks like we have a similar issues here as well (ciInstanceKlass::compute_nonstatic_fields_impl):
>>>> http://hg.openjdk.java.net/valhalla/valhalla/file/448c8cd077c9/src/hotspot/share/ci/ciInstanceKlass.cpp#l494
>>>> It just expands nested flattened fields by one level.
>>> I don't think that's true. That code iterates over the fields of a flattened value klass by using
>>> nof_nonstatic_fields() and nonstatic_field_at() which contains the flattened fields of that klass as
>>> well:
>>> http://hg.openjdk.java.net/valhalla/valhalla/file/448c8cd077c9/src/hotspot/share/ci/ciInstanceKlass.cpp#l530
>>>
>>> That code is heavily used by C2, hopefully we would have noticed if it's incorrect.
>>>
>> Hmmm .... Both ciInstanceKlass::compute_nonstatic_fields_impl and Fred's new GraphBuilder::copy_value_content have the same loop:
>>
>>        for (int i = 0; i < vk->nof_nonstatic_fields(); ++i) {
>>          ciField* xxx = vk->nonstatic_field_at(i);
>>
>> Why do we need to recurse only in the second loop but not the first? It seems like the first loop assumes that vk->nof_nonstatic_fields() has already been recursively flattened, but the second loop doesn't.
>>
>> I added the two asserts below and they never failed when running all the compiler tests with C2.
>>
>> I can also pass TestBasicFunctionality.test26 with Fred's patch (and my assert).
>>
>>
>> I think this means other parts of Fred's patch already fixes the recursive flattening issue, and we don't need the recursive copy in GraphBuilder::copy_value_content
>>
>> Thanks
>> - Ioi
>>
>>
>>
>>
>> ciInstanceKlass::compute_nonstatic_fields_impl() {
>>        ....
>>        for (int i = 0; i < vk->nof_nonstatic_fields(); ++i) {
>>          ciField* flattened_field = vk->nonstatic_field_at(i);
>>          assert(!flattened_field->is_flattened(), "must be"); /// <<<< added by Ioi
>>          // Adjust offset to account for missing oop header
>>          int offset = field_offset + (flattened_field->offset() - vk->first_field_offset());
>>          // A flattened field can be treated as final if the non-flattened
>>          // field is declared final or the holder klass is a value type itself.
>>          bool is_final = fd.is_final() || is_valuetype();
>>          ciField* field = new (arena) ciField(flattened_field, this, offset, is_final);
>>          fields->append(field);
>>        }
>>
>>
>> --- vs ---
>>
>> void GraphBuilder::copy_value_content(ciValueKlass* vk, Value src, int src_off, Value dest, int dest_off,
>>      ValueStack* state_before, bool needs_patching) {
>>    for (int i = 0; i < vk->nof_nonstatic_fields(); i++) {
>>      ciField* inner_field = vk->nonstatic_field_at(i);
>>      int off = inner_field->offset() - vk->first_field_offset();
>>      assert(!inner_field->is_flattened(), "must be");   /// <<<< added by Ioi
>>      if (inner_field->is_flattened()) {
>>        assert(inner_field->type()->is_valuetype(), "Sanity check");
>> copy_value_content(inner_field->type()->as_value_klass(), src, src_off + off, dest, dest_off + off,
>>            state_before, needs_patching);
>>      } else {
>>        LoadField* load = new LoadField(src, src_off + off, inner_field, false, state_before, needs_patching);
>>        Value replacement = append(load);
>>        StoreField* store = new StoreField(dest, dest_off + off, inner_field, replacement, false, state_before, needs_patching);
>>        append(store);
>>      }
>>    }
>> }


More information about the valhalla-dev mailing list