RFR: 8260355: AArch64: deoptimization stub should save vector registers

[Vladimir Ivanov]

On Thu, 28 Jan 2021 08:27:22 GMT, Nick Gasson <ngasson at openjdk.org> wrote:

> This is an AArch64 port of the fix for JDK-8256056 "Deoptimization stub
> doesn't save vector registers on x86". The problem is that a vector
> produced by the Vector API may be stored in a register when the deopt
> blob is called. Because the deopt blob only stores the lower half of
> vector registers, the full vector object cannot be rematerialized during
> deoptimization. So the following will crash on AArch64 with current JDK:
> 
>   make test TEST="jdk/incubator/vector" \
>     JTREG="VM_OPTIONS=-XX:+DeoptimizeALot -XX:DeoptimizeALotInterval=0"
> 
> The fix is to store the full vector registers by passing
> save_vectors=true to save_live_registers() in the deopt blob. Because
> save_live_registers() places the integer registers above the floating
> registers in the stack frame, RegisterSaver::r0_offset_in_bytes() needs
> to calculate the SP offset based on whether full vectors were saved, and
> whether those vectors were NEON or SVE, rather than using a static
> offset as it does currently.
> 
> The change to VectorSupport::allocate_vector_payload_helper() is
> required because we only store the lowest VMReg slot in the oop map.
> However unlike x86 the vector registers are always saved in a contiguous
> region of memory, so we can calculate the address of each vector element
> as an offset from the address of the first slot. X86 handles this in
> RegisterMap::pd_location() but that won't work on AArch64 because with
> SVE there isn't a unique VMReg corresponding to each four-byte physical
> slot in the vector (there are always exactly eight logical VMRegs
> regardless of the actual vector length).
> 
> Tested hotspot_all_no_apps and jdk_core.

Much better, thanks.

I suggest the following changes:

* please, leave original `location(VMReg reg)`/`pd_location(VMReg reg)` intact and introduce `(VMReg reg, uint slot_idx)` overloads;

* it's fair for `location(VMReg reg, uint slot_idx)` to require `reg` to  always be a base register:
address RegisterMap::location(VMReg base_reg, uint slot_idx) const {
  if (slot_idx > 0) {
    return pd_location(base_reg, slot_idx);
  } else {
    return location(base_reg);
}

* on all platforms except AArch64 define `pd_location(VMReg base_reg, int slot_idx)` as:
address RegisterMap::pd_location(VMReg base_reg, int slot_idx) const {
  return location(base_reg->next(slot_idx));
}

* on AArch64 check for vector register case and special-case it to:
address RegisterMap::pd_location(VMReg base_reg, int slot_idx) const {
  if (base_reg->is_FloatRegister()) {
    assert((base_reg->value() - ConcreteRegisterImpl::max_gpr) % FloatRegisterImpl::max_slots_per_register == 0, "not a base register");
    intptr_t offset_in_bytes = slot_idx * VMRegImpl::stack_slot_size;
    address base_location = location(base_reg);
    if (base_location != NULL) {
      return base_location + offset_in_bytes;
    }
  }
  return location(base_reg->next(slot_idx));
}
 

Or, as an alternative (since all the registers are stored contiguously on AArch64 anyway):
address RegisterMap::pd_location(VMReg base_reg, int slot_idx) const {
  intptr_t offset_in_bytes = slot_idx * VMRegImpl::stack_slot_size;
  address base_location = location(base_reg);
  if (base_location != NULL) {
    return base_location + offset_in_bytes;
  }
}

* keep the assert in `VMReg::next(int)` if you wish, but refactor it into an out-of-bounds check instead. Personally, I'd prefer to see it as a separate enhancement.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2279