[aarch64-port-dev ] RFR(S/M): 8247766: [aarch64] guarantee(val < (1U << nbits)) failed: Field too big for insn

[Patric Hedlin]

Hi Andrew,

On 2020-07-09 17:48, Andrew Haley wrote:
> On 07/07/2020 12:17, Patric Hedlin wrote:
>> Dear all,
>>
>> I would like to ask for help to review the following change/update:
>>
>> Issue:  https://bugs.openjdk.java.net/browse/JDK-8247766
>> Webrev: http://cr.openjdk.java.net/~phedlin/tr8247766/
>>
>>
>> C1 code generation for reading and writing stack-slots does not handle
>> large immediate offsets on aarch64. This patch will ensure that
>> immediate offsets are admissible for base+(immediate)offset encoding or,
>> if this is not the case, will enforce an explicit address calculation to
>> a scratch register. (Also correcting a small glitch in 9-bit signed
>> immediate encoding check.)
> This is all very complicated.
>
> So it seems to me that there is a better way to do this. We already have
> MacroAssembler::legitimize_address(), and you should use that.
>
> Like so:
>
> diff -r 7c59af4db158 src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp
> --- a/src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp       Thu Jul 09 11:01:29 2020 -0400
> +++ b/src/hotspot/cpu/aarch64/c1_LIRAssembler_aarch64.cpp       Thu Jul 09 11:36:02 2020 -0400
> @@ -736,25 +736,32 @@
>
>   void LIR_Assembler::reg2stack(LIR_Opr src, LIR_Opr dest, BasicType type, bool pop_fpu_stack) {
>     if (src->is_single_cpu()) {
> +    int index = dest->single_stack_ix();
>       if (is_reference_type(type)) {
> -      __ str(src->as_register(), frame_map()->address_for_slot(dest->single_stack_ix()));
> +      __ str(src->as_register(),
> +             __ legitimize_address(frame_map()->address_for_slot(index), BytesPerWord, rscratch1));
>         __ verify_oop(src->as_register());
>       } else if (type == T_METADATA || type == T_DOUBLE || type == T_ADDRESS) {
> -      __ str(src->as_register(), frame_map()->address_for_slot(dest->single_stack_ix()));
> +      __ str(src->as_register(),
> +             __ legitimize_address(frame_map()->address_for_slot(index), BytesPerWord, rscratch1));
>       } else {
> -      __ strw(src->as_register(), frame_map()->address_for_slot(dest->single_stack_ix()));
> +      __ strw(src->as_register(),
> +              __ legitimize_address(frame_map()->address_for_slot(index), BytesPerInt, rscratch1));
>       }
>
>     } else if (src->is_double_cpu()) {
>       Address dest_addr_LO = frame_map()->address_for_slot(dest->double_stack_ix(), lo_word_offset_in_bytes);
> +    dest_addr_LO = __ legitimize_address(dest_addr_LO, BytesPerLong, rscratch1);
>       __ str(src->as_register_lo(), dest_addr_LO);
>
>     } else if (src->is_single_fpu()) {
>       Address dest_addr = frame_map()->address_for_slot(dest->single_stack_ix());
> +    dest_addr = __ legitimize_address(dest_addr, BytesPerInt, rscratch1);
>       __ strs(src->as_float_reg(), dest_addr);
>
>     } else if (src->is_double_fpu()) {
>       Address dest_addr = frame_map()->address_for_slot(dest->double_stack_ix());
> +    dest_addr = __ legitimize_address(dest_addr, BytesPerLong, rscratch1);
>       __ strd(src->as_double_reg(), dest_addr);
>
>     } else {
>
> stack_offset_in_reach() seems to duplicate the functionality of offset_ok_for_immed(),
> and it's only used in this one place. By all means please use the new is_uimm() and
> is_simm() in offset_ok_for_immed().
>

I've refreshed the webrev (as discussed off-line), moving 
legitimize_address() into the stack_slot_address() with additional 
conditions related to the (well-aligned) frame slot address produced. 
Use of is_simm9() and is_uimm12() is now using implementation in Assembler.

/Patric