[aarch64-port-dev ] 8233948: AArch64: Incorrect mapping between OptoReg and VMReg for high 64 bits of Vector Register

[Andrew Dinn]

Hi Joshua,

Thanks for looking into this and suggesting the required cleanup.

On 15/11/2019 10:29, Joshua Zhu (Arm Technology China) wrote:
>> Please review the following patch:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8233948
>> Webrev: http://cr.openjdk.java.net/~jzhu/8233948/webrev.00/
> 
> Please let me know if any comments. Thanks a lot.
I think this is a good start but there is more work to do to clean up
method RegisterSaver::save_live_registers defined in file
sharedRuntime_aarch64.cpp. It would be good to do that clean up as part
of this patch so it is all consistent.

So, the first step is to add a couple of extra enum constants in
FloatRegisterImpl:

 128 class FloatRegisterImpl: public AbstractRegisterImpl {
 129  public:
 130   enum {
 131     number_of_registers = 32,
 132     max_slots_per_register = 4,
         save_slots_per_register = 2,
         extra_save_slots_per_register = 2

The 2 new tags are needed because sharedRuntime_aarch64.cpp normally
only saves 2 slots per register but it occasionally needs to save all 4.

The first bit of code in sharedRuntime_aarch64.cpp that needs fixing is
this enum:

 100   enum layout {
 101                 fpu_state_off = 0,
 102                 fpu_state_end = fpu_state_off+FPUStateSizeInWords-1,
 103                 // The frame sender code expects that rfp will be in
 104                 // the "natural" place and will override any oopMap
 105                 // setting for it. We must therefore force the layout
 106                 // so that it agrees with the frame sender code.
 107                 r0_off = fpu_state_off+FPUStateSizeInWords,
 108                 rfp_off = r0_off + 30 * 2,
 109                 return_off = rfp_off + 2,      // slot for return
address
 110                 reg_save_size = return_off + 2};

This information defines the layout of the data normally saved to stack
(i.e. 2 slots per fp reg). These values should really be computed using
the enum values you added to the definitions for RegisterImpl and
FloatRegisterImpl.

FPUStateSizeInWords is actually defined in assembler.hpp.  It doesn't
really need to be there but we put it there to follow the logic for x86
where the amount of saved state is more complicated. The AArch64
definiton at assembler.hpp:607 is this:

 607   const int FPUStateSizeInWords = 32 * 2;

So, that can now be redefined as

 607   const int FPUStateSizeInWords =
FloatRegisterImpl::number_of_registers *
FloatRegisterImpl::save_slots_per_register;

We then need to redefine the code at lines 108 - 110 to use the enum values:

 108                 rfp_off = r0_off +
(RegisterImpl::number_of_registers - 2) *
RegisterImpl::max_slots_per_register,
 109                 return_off = rfp_off +
RegisterImpl::max_slots_per_register,      // slot for return address
 110                 reg_save_size = return_off +
RegisterImpl::max_slots_per_register};

Finally, we can method edit save_live_registers at the point where it
allows space for the extra vector register content. That needs to be
updated to use the relevant constants:

 116   if (save_vectors) {
 117     // Save upper half of vector registers
 118     int vect_words = FloatRegisterImpl::number_of_registers *
FloatRegisterImpl::extra_save_slots_per_register;
 119     additional_frame_words += vect_words;

Could you prepare a new webrev with these extra changes in and check it
is ok?

Also, could you report what testing you did before and after your change
(other than checking the dump output). You will probably need to repeat
it to ensure these extra changes are ok.

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill