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

[Frederic Parain]

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);
>     }
>   }
> }