RFR 8220788 [lworld] C1 support for LW2 Arrays

[Ioi Lam]

Hi Tobias,

Thanks for the review. I've updated the patch. Here's the delta from the 
last webrev:

http://cr.openjdk.java.net/~iklam/valhalla/8220788-c1-support-for-lw2-arrays.v02-delta/

On 5/21/2019 1:05 AM, Tobias Hartmann wrote:
> Hi Ioi,
>
> looks good to me, nice progress! Here are some comments/questions:
>
> c1_Instruction.cpp:
> - line 151: no need to check for is_null_free()

Removed.

> - line 299: I think you can remove the else branch because we return false anyway and maybe add the
> comment to before the check in line 297 ("this can fail with inling ..")

I changed it to:

     // The following check can fail with inlining:
     //     void test45_inline(Object[] oa, Object o, int index) { 
oa[index] = o; }
     //     void test45(MyValue1[] va, int index, MyValue2 v) { 
test45_inline(va, v, index); }
     if (element_klass == actual_klass) {
       return true;
     }


> c1_LIRAssembler_x86.cpp:
> - line 1934: The comment is a bit misleading because we are also emitting this check for interface
> arrays and MyValue?[], right?

I changed to

   // We are loading/storing an array that *may* be a flattened array 
(the declared type
   // Object[], interface[], or VT?[]). If this array is flattened, take 
slow path.


> - line 1940: Why do you need that check? And why do you still emit the flattened check even if
> value()->is_illegal()?

emit_opFlattenedArrayCheck is called for both aaload and aastore. In the 
case of aaload, op->value() is illegal. See LIRGenerator::do_LoadIndexed

       check_flattened_array(array.result(), LIR_OprFact::illegalOpr, 
slow_path);

I use emit_opFlattenedArrayCheck for both aaload and aastore because 
it's a lot of hassle to write a new LIR_Op.

> - line 1956: "actual array is a "QLVT;" should be "actual array is a "[QVT;", right?

Fixed.

> macroAssembler_x86.cpp:
> - Why is that change necessary?

There were two places in c1_LIRAssembler_x86.cpp that called 
decode_klass_not_null directly.

In my updated patch: For LIR_Assembler::arraycopy_flat_check, I rewrote 
it to avoid loading the klass altogether. For LIR_Assembler::mem2reg, I 
added the masking operation before calling decode_klass_not_null.

Anyway, I think the masking operation should be placed inside 
decode_klass_not_null. Maybe I'll fix that in a subsequent RFE.

Thanks
- Ioi


> Thanks,
> Tobias
>
> On 20.05.19 05:03, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8220788
>> http://cr.openjdk.java.net/~iklam/valhalla/8220788-c1-support-for-lw2-arrays.v01/
>>
>> With this changeset, C1 can properly distinguish between "[QV;" and "[LV;" arrays. It can also
>> handle non-flattened arrays that are null free (e.g., "[V;" arrays where the size of V is larger
>> than ValueArrayElemMaxFlatSize).
>>
>> Testing:
>>
>>      cd test/hotspot/jtreg/compiler/valhalla/valuetypes
>>      jtreg -vmoptions:-XX:+EnableValhallaC1 \
>>            -vmoptions:-XX:TieredStopAtLevel=1 \
>>            -vmoptions:-XX:-ValueTypePassFieldsAsArgs \
>>            -vmoptions:-XX:-ValueTypeReturnedAsFields \
>>            -Dtest.c1=true \
>>            .
>>
>> All tests passed.
>>
>> My next step is to test C1 more thoroughly, and add test groups for C1 under valhalla-hs-tier* for
>> mach5 testing.
>>
>> Thanks
>> - Ioi
>>
>>