RFR 8220788 [lworld] C1 support for LW2 Arrays

[Tobias Hartmann]

Hi Ioi,

thanks for the explanations, looks good to me!

Best regards,
Tobias

On 22.05.19 18:10, Ioi Lam wrote:
> 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
>>>
>>>
>