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

[Andrew Haley]

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().

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671