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

[Tobias Hartmann]

Hi Ioi,

thanks for making these changes, the new webrev looks good to me.

Best regards,
Tobias

On 12.03.19 06:31, Ioi Lam wrote:
> 
> 
> 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
>>>
>