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

[Joshua Zhu (Arm Technology China)]

Hi Andrew Dinn,

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

Thanks for your review!
 
>  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.

Make sense.
Bottom 64 bits of SVE vector register still overlaps with floating-point register,
Therefore I will make 
    extra_save_slots_per_register = max_slots_per_register - save_slots_per_register;
so that we just need to improve the value of max_slots_per_register for SVE in future.

>  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};

Thanks for your reminder! Make sense.
 
> 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;

Here the computation of vect_words should be:
    int vect_words = FloatRegisterImpl::number_of_registers * FloatRegisterImpl::extra_save_slots_per_register /
                                   VMRegImpl::slots_per_word;

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

Yes, of course. I updated webrev according to your review comments.
http://cr.openjdk.java.net/~jzhu/8233948/webrev.01/

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

Oh, Sorry for that. I did not mention my testing in my previous mail (My initial RFR mail is too long).
I ran full jtreg tests without new failure introduced.
I will re-run it against the new webrev and will let you know the results.
Thanks again for your review. 

Best Regards,
Joshua