RFR(S): 8159611: C2: ArrayCopy elimination skips required parameter checks
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Aug 26 01:51:54 UTC 2016
Looks good.
Check does not fold because it is different: LT vs LE.
Actually there are 3 checks together with yours (see
PhaseMacroExpand::generate_arraycopy()):
Node* not_pos = generate_nonpositive_guard(ctrl, copy_length,
length_never_negative);
if (not_pos != NULL) {
Node* local_ctrl = not_pos, *local_io = *io;
MergeMemNode* local_mem = MergeMemNode::make(mem);
transform_later(local_mem);
// (6) length must not be negative.
if (!length_never_negative) {
generate_negative_guard(&local_ctrl, copy_length, slow_region);
}
I think the only way to avoid this is to modify code in
generate_arraycopy() when EliminateAllocations is true. In such case you
need to generate only length == 0 check.
Thanks,
Vladimir
On 8/25/16 10:03 AM, Volker Simonis wrote:
> On Tue, Aug 16, 2016 at 11:49 PM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com> wrote:
>> Not generating exception is definitely bug.
>>
>> First, about test case. It would be nice if it also verifies other
>> IndexOutOfBoundsException cases.
>>
>
> I've extended the test case. See:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8159611.v2/
>
> With the new test I've caught another problem in C1 (only on x86 and
> s390, but that's not in the OpenJDK yet :).
>
> LIR_Assembler::emit_arraycopy() had a shortcut for length==0 which
> prevented the throwing of an ArrayStoreException if src and dst arrays
> have incompatible type (see do_test2() in the new regression test).
> Note that this is a different error from 8160591 and not fixed by the
> change for 8160591.
>
> I've also moved the new check after the offset + length check as
> suggested by you (see new webrev).
>
> Unfortunately, the new check is still not eliminated. Here's how it looks:
>
> 0ae B6: # B20 B7 <- B5 Freq: 0,999997
> 0ae movl R9, [rsp + #0] # spill
> 0b2 testl R9, R9
> 0b5 jl B20 P=0,000001 C=-1,000000
> 0b5
> 0bb B7: # B12 B8 <- B6 Freq: 0,999996
> 0bb movl R11, [R10 + #8 (8-bit)] # compressed klass ptr
> 0bf decode_klass_not_null RAX,R11
> 0cc movl RBX, [RAX + #16 (8-bit)] # int
> 0cf movslq RCX, RBX # i2l
> 0d2 movq RSI, precise klass [Ljava/lang/Object;:
> 0x00007ff1080320d0:Constant:exact * # ptr
> 0dc movq RCX, [RSI + RCX] # class
> 0e0 cmpq RAX, RCX # ptr
> 0e3 jne,us B12 P=0,170000 C=-1,000000
> 0e3
> 0e5 B8: # B21 B9 <- B7 B13 B14 Freq: 0,999996
> 0e5 testl R9, R9
> 0e8 jle B21 P=0,000001 C=-1,000000
>
> As you can see 'testl R9, R9' is executed two times.
>
> I've even tried to move the new check after the subtype check, but
> that doesn't helps either:
>
> 0da B7: # B20 B8 <- B6 B13 B14 Freq: 0,999997
> 0da movl R11, [rsp + #8] # spill
> 0df testl R11, R11
> 0e2 jl B20 P=0,000001 C=-1,000000
> 0e2
> 0e8 B8: # B10 B9 <- B7 Freq: 0,999996
> 0e8 testl R11, R11
> 0eb jle,s B10 P=0,000001 C=-1,000000
>
> Any idea how this could be fixed?
>
> Thanks,
> Volker
>
> PS: and I still don't have a reproducible benchmark which shows a
> regression with my change...
>
>
>> Actually additional dynamic check will help in case of negative length is
>> know during compilation. The allocation code will be eliminated very early
>> instead of waiting macro expansion:
>>
>> int length = alloc->in(AllocateNode::ALength)->find_int_con(-1);
>> if (length < 0) {
>> NOT_PRODUCT(fail_eliminate = "Array's size is not constant";)
>> can_eliminate = false;
>> }
>>
>> About additional length check in your new test. I think it may be collapsed
>> with preceding check since it is generated after other checks.
>> So I would suggest to move it after offset + length check.
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 8/16/16 7:57 AM, Volker Simonis wrote:
>>>
>>> On Tue, Aug 16, 2016 at 7:24 AM, Tobias Hartmann
>>> <tobias.hartmann at oracle.com> wrote:
>>>>
>>>> Hi Volker,
>>>>
>>>> thanks for taking care of this issue!
>>>>
>>>> Did you check what happens if the allocation is not eliminated and macro
>>>> expansion phase emits another negative guard? Are the checks merged?
>>>>
>>>
>>> It depends. I just saw that in some cases the regression test worked
>>> before, because the length check was done in
>>> SharedRuntime::slow_arraycopy_C(). So in that case there's obviously
>>> nothing that can be merged. But the test case is obviously a
>>> degenerated example anyway, so I don't think that's a problem.
>>>
>>> If I do a more real-world example like this where the arracopy can not
>>> be eliminated because one of its arguments escapes:
>>>
>>> public static boolean do_test2(int length, Object[] dest) {
>>> try {
>>> System.arraycopy(new Object[10], 1, dest, 1, length);
>>> return false;
>>> } catch (IndexOutOfBoundsException e) {
>>> return true;
>>> }
>>> }
>>>
>>> and call it with:
>>>
>>> do_test2(8, new Object[10])
>>>
>>> the generated code for do_test2() unfortunately contains one more
>>> check now with my change (the 'length' field is in [rsp + #0]):
>>>
>>> 0a2 B4: # B18 B5 <- B3 Freq: 0,999999
>>> 0a2 movl R9, [rsp + #0] # spill
>>> 0a6 testl R9, R9
>>> 0a9 jl B18 P=0,000001 C=-1,000000
>>> 0a9
>>> 0af B5: # B18 B6 <- B4 Freq: 0,999998
>>> 0af movl RBX, R9 # spill
>>> 0b2 incl RBX # int
>>> 0b4 cmpl RBX, #10 # unsigned
>>> 0b7 jnbe,u B18 P=0,000001 C=-1,000000
>>>
>>> The generated code before my change looked like this (againthe
>>> 'length' field is in [rsp + #0]):
>>>
>>> 0a1 B4: # B17 B5 <- B3 Freq: 0,999999
>>> 0a1 movl R11, [rsp + #8] # spill
>>> 0a6 incl R11 # int
>>> 0a9 cmpl R11, #10 # unsigned
>>> 0ad jnbe,u B17 P=0,000001 C=-1,000000
>>>
>>> It seems that the 'length' check has been completely eliminated before.
>>>
>>> So I need to do some more tests to understand why the new check isn't
>>> eliminated.
>>>
>>> Do you think the new check results in a performance regression? Have
>>> you run some benchmarks?
>>>
>>>> I would prefer brackets around the if body but you don't need to send
>>>> another webrev:
>>>> if (EliminateAllocations) {
>>>> generate_negative_guard(length, slow_region);
>>>> }
>>>
>>>
>>> Yes, I agree.
>>>
>>>>
>>>> Best regards,
>>>> Tobias
>>>>
>>>> On 12.08.2016 21:13, Volker Simonis wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> can I please have a review and sponsor for the following fix:
>>>>>
>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8159611
>>>>> https://bugs.openjdk.java.net/browse/JDK-8159611
>>>>>
>>>>>
>>>>> We are inserting several checks for the arguments of
>>>>> System.arraycopy() in LibraryCallKit::inline_arraycopy() before
>>>>> intensifying the call in LibraryCallKit::inline_arraycopy. However the
>>>>> check for the 'length' argument of arracopy is postponed to the macro
>>>>> expansion phase in PhaseMacroExpand::generate_arraycopy().
>>>>>
>>>>> But if we are running with EscapeAnalysis and EliminateAllocations,
>>>>> the array allocations inside a call to System.arraycopy() may get
>>>>> eliminated and thus the complete call to System.arraycopy() will be
>>>>> removed (see PhaseMacroExpand::process_users_of_allocation). In this
>>>>> case the extra 'length' check won't be added by
>>>>> PhaseMacroExpand::generate_arraycopy() any more because macro
>>>>> expansion happens after the elimination of macro nodes.
>>>>>
>>>>> In such a case it may happen that System.arraycopy() will silently
>>>>> accept an invalid (i.e. negative) 'length' parameter, although it
>>>>> should actually throw an ArrayOutOfBounds exception.
>>>>>
>>>>> The fix is simple: also insert a check for the length field in
>>>>> LibraryCallKit::inline_arraycopy() if we are running with
>>>>> EliminateAllocations.
>>>>>
>>>>> Regards,
>>>>> Volker
>>>>>
>>
More information about the hotspot-compiler-dev
mailing list