RFR(S): 8159611: C2: ArrayCopy elimination skips required parameter checks

[Volker Simonis]

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
>>