RFR 8229382 [lworld][c1] TestLWorld.java crashes with -XX:+UseSerialGC

[Ioi Lam]

Hi Tobias,

Thanks for the review.

I found a bug in the patch. LIRGenerator::load_constant() should look 
like this:

     if (!in_conditional_code()) {
       _constants.append(c);
+     _reg_for_constants.append(result);
     }
-   _reg_for_constants.append(result);

Now all valhalla tests passed with C1 and -XX:+UseSerialGC. I will do 
more testing before pushing.

Thanks
- Ioi



On 8/13/19 10:59 PM, Tobias Hartmann wrote:
> Hi Ioi,
>
> good catch! Looks good to me.
>
> Thanks,
> Tobias
>
> On 13.08.19 05:50, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8229382
>> http://cr.openjdk.java.net/~iklam/valhalla/8229382-c1-dont-cache-constants-in-conditional-code.v01/
>>
>> Background
>> ----------
>>
>> When c1_LIRGenerator.cpp translates the HIR of a basic block to LIR, and it
>> loads a constant C into a virtual register R, it assumes that this must happen
>> for all possible paths through this basic block. Thus, it uses a cache
>> of {constant} -> {register} to avoid loading the same constant C again
>> in the same basic block. See LIRGenerator::load_constant().
>>
>> However, when translating aastore, we end up having code like this:
>>
>>   0x7f46c544a260: test $0x1,%bl            ; is this array flattened?
>>   0x7f46c544a263: jne 0x7f46c544a376       ; call store_flattened_array
>>   ....
>>   0x7f46c544a28b: mov $0x7f03335dc000,%rsi ; card_table_base
>>   0x7f46c544a295: movb $0x0,(%rax,%rsi,1)  ; mark card table
>>
>> so the constant card_table_base is stored into %rsi only if the array
>> is not flattened.
>>
>> When a subsequent aastore is translated, we mistakenly assume that
>> %rsi already contains the desired value. The code will crash when
>> using %rsi to mark the card table.
>>
>>
>> Fix
>> ---
>>
>> The fix is to avoid caching constants used in conditionally executed LIR.
>>
>> I also added assertions so we don't call LIRItem::load_item() while
>> generating conditionally executed LIR. Otherwise we will create
>> {Value} -> {LIR_Opr} mappings (i.e., from HIR values to LIR virtual
>> registers) that are not always valid.
>>
>> I looked at every line of "__ branch_destination" added by the
>> valhalla code and hopefully have caught all the bugs introduced by me.
>>
>> Thanks
>> - Ioi
>>