RFR 8220269 [lworld] C1 should return default value for uninitialized non-static Q fields

[Ioi Lam]

On 3/8/19 1:32 AM, Tobias Hartmann wrote:
> Hi Ioi,
>
> overall look good, here are some comments:
> - Why do you specify -XX:+Inline in the test? That should be the default. I think the test should
> also be executed with -XX:ValueFieldMaxFlatSize=0.
I thought some of the methods need to be forced to be inlined, so I 
added -XX:+Inline. But as you said, that's not the right way to do it. 
The correct way is to convert the test to be a subclass of 
compiler.valhalla.valuetypes.ValueTypeTest and use the @ForceInline 
annotation.

So I converted the test, and also added a scenario with 
-XX:ValueFieldMaxFlatSize=0. Both C1 and C2 passed.

(It turned out I didn't need the @ForceInline annotations).

> - The comment in c1_LIRGenerator.cpp:1893 is a bit confusing because there is no check. Maybe add an
> assert?

I wanted to enumerate all 4 possibilities to make sure we checked all 
the corner cases:

   (0) static field or not
   (1) is holder class loaded
   (2) is field type loaded
   (3) is field flattened

That's why I left (1) and (3) under the "if (field->is_static())" 
branch. To clarify, I added another comment "// No check needed here."

> - c1_LIRGenerator.cpp:1901: "don't whether" -> "don't know whether"
> - c1_LIRGenerator.cpp:1909: This assert can be simplified and moved into the first if branch (1912)
Fixed.
> - There is a difference between flattened and flattenable (see corresponding ciField methods). It
> seems like you are using the terms interchangeably in the comments/code.
I changed "flattenable" in the comments to "flattened", which is what 
the code needs.

> - Can you use is_flattenable() instead of is_q_type() or does that not work for unloaded klasses?
I changed the code to use is_flattenable(), and removed is_q_type(). I 
had to fix a bug where ciField::_is_flattenable is uninitialized for 
unloaded classes.

http://cr.openjdk.java.net/~iklam/valhalla/8220269-getfield-uninited-jumbo-qfield.v03/
http://cr.openjdk.java.net/~iklam/valhalla/8220269-getfield-uninited-jumbo-qfield.v03-delta/


Thanks
- Ioi

> Thanks,
> Tobias
>
> On 08.03.19 04:57, Ioi Lam wrote:
>>
>> On 3/6/19 9:32 PM, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8220269
>>> http://cr.openjdk.java.net/~iklam/valhalla/8220269-getfield-uninited-jumbo-qfield.v01/
>>>
>>> For the following bytecode
>>>
>>>      getfield Holder.f:"QJumboValue"
>>>
>>> If the field is too big to be flattened (e.g., ciField::is_flattened()
>>> returns false), we should load the field as an oop, test for null, and
>>> replace with JumboValue.default.
>>>
>>> ---
>>>
>>> I also refactored the handling of unloaded classes by getfield/getstatic
>>> into a new function LIRGenerator::non_nullable_load_field_prolog(). I needed to
>>> add a new method:
>>>
>>>    bool ciField::is_never_null() const {
>>>      // Cannot use (type()->basic_type() == T_VALUETYPE) -- if the class is
>>>      // not loaded, type() is an unloaded ciInstanceKlass!
>>>      return signature()->char_at(0) == 'Q';
>>>    }
>>>
>> Per John Rose's comments (offline), I renamed this function to ciField::is_q_type(), as there's a
>> chance in the future that "Q" doesn't necessary mean "not nullable".
>>
>>
>>> I think putfield also need to be fixed, because it currently rely on
>>> ciField::type(), which essentially changes "Q" to "L" for unloaded value classes.
>>>
>>> But I will do that in a separate JBS issue.
>>>
>> I double checked the code and the tests, and we already correctly handle the case where putfield is
>> used with an unloaded valuetype. The test cases are TestUnloadedValueTypeField::test{4,5}. I
>> corrected the comments and and added an assert in LIRGenerator::do_StoreField().
>>
>> Delta from the last webrev
>>
>> http://cr.openjdk.java.net/~iklam/valhalla/8220269-getfield-uninited-jumbo-qfield.v02-delta/
>>
>> Thanks
>> - Ioi
>>