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

[Ioi Lam]

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