[aarch64-port-dev ] Assert failure building Graal

[Stuart Monteith]

Hello,
  I think I prefer what you have there Andrew, rather than pushing
onto the stack. I guess the mistake was when using r22 and r23 was our
not saving them as callee-saved registers. There is nothing on the
path from RangeCheckStub to the exception throw that overwrote r22 or
r23 (AFAICT from stepping through the code), and the code does return.

r8 and r9 have no opportunities to be overwritten between them being
set and used, so it'll be ok.

Shall I raise a bug and suggest your change be merged?

BR,
    Stuart

On 7 June 2018 at 12:22, Andrew Haley <aph at redhat.com> wrote:
> On 06/07/2018 11:17 AM, Stuart Monteith wrote:
>> I've retested with them removed, and the tests run to completion. It
>> appears my suggestion for using r22/r23 instead of the scratch
>> registers weren't quite sufficient to be error free.
>
> That's right: everything is live.  This is a very tough nut to crack, but
> I think this will do it:
>
> diff -r b06f330492cd src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp
> --- a/src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp  Wed Jun 06 13:06:21 2018 +0100
> +++ b/src/hotspot/cpu/aarch64/c1_CodeStubs_aarch64.cpp  Thu Jun 07 12:15:42 2018 +0100
> @@ -73,19 +73,20 @@
>    }
>
>    if (_index->is_cpu_register()) {
> -    __ mov(r22, _index->as_register());
> +    __ mov(rscratch1, _index->as_register());
>    } else {
> -    __ mov(r22, _index->as_jint());
> +    __ mov(rscratch1, _index->as_jint());
>    }
>    Runtime1::StubID stub_id;
>    if (_throw_index_out_of_bounds_exception) {
>      stub_id = Runtime1::throw_index_exception_id;
>    } else {
>      assert(_array != NULL, "sanity");
> -    __ mov(r23, _array->as_pointer_register());
> +    __ mov(rscratch2, _array->as_pointer_register());
>      stub_id = Runtime1::throw_range_check_failed_id;
>    }
> -  __ far_call(RuntimeAddress(Runtime1::entry_for(stub_id)), NULL, rscratch2);
> +  __ lea(lr, RuntimeAddress(Runtime1::entry_for(stub_id)));
> +  __ blr(lr);
>    ce->add_call_info_here(_info);
>    ce->verify_oop_map(_info);
>    debug_only(__ should_not_reach_here());
> diff -r b06f330492cd src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp
> --- a/src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp   Wed Jun 06 13:06:21 2018 +0100
> +++ b/src/hotspot/cpu/aarch64/c1_Runtime1_aarch64.cpp   Thu Jun 07 12:15:42 2018 +0100
> @@ -323,7 +323,7 @@
>
>
>  // target: the entry point of the method that creates and posts the exception oop
> -// has_argument: true if the exception needs arguments (passed in r22 and r23)
> +// has_argument: true if the exception needs arguments (passed in rscratch1 and rscratch2)
>
>  OopMapSet* Runtime1::generate_exception_throw(StubAssembler* sasm, address target, bool has_argument) {
>    // make a frame and preserve the caller's caller-save registers
> @@ -332,7 +332,9 @@
>    if (!has_argument) {
>      call_offset = __ call_RT(noreg, noreg, target);
>    } else {
> -    call_offset = __ call_RT(noreg, noreg, target, r22, r23);
> +    __ mov(c_rarg1, rscratch1);
> +    __ mov(c_rarg2, rscratch2);
> +    call_offset = __ call_RT(noreg, noreg, target);
>    }
>    OopMapSet* oop_maps = new OopMapSet();
>    oop_maps->add_gc_map(call_offset, oop_map);
>
> An alternative would be to push the array and the index onto the stack
> and then write a custom version of save_live_registers() which saves
> everything and pops the array and the index into r1 and r2, leaving
> exactly the same stack layout.  That's more work, though, and not
> obviously any better.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671