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

Tobias Hartmann tobias.hartmann at oracle.com
Wed Nov 28 09:41:01 UTC 2018


Good catch Ioi, I've missed that in my first review. Looks good!

Best regards,
Tobias

On 27.11.18 21:55, 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