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

[Tobias Hartmann]

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.
- The comment in c1_LIRGenerator.cpp:1893 is a bit confusing because there is no check. Maybe add an
assert?
- 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)
- 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.
- Can you use is_flattenable() instead of is_q_type() or does that not work for unloaded klasses?

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
>