RFR: 8351666: [PPC64] Make non-volatile VectorRegisters available for C2 register allocation [v4]

Wed Apr 9 20:52:34 UTC 2025

On Tue, 8 Apr 2025 16:08:32 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> This PR makes the non-volatile VectorRegisters available for C2's register allocation.
>> 
>> I had to implement the VectorRegisters properly (4 VM Regs) like on other platforms. The old version has run into assertions and looked strange.
>> 
>> The non-volatile VectorRegisters are now saved when entering Java: call_stub and upcall_stubs.
>> I have rewritten the save and restore functions and used them for both. Then, I have removed code which has become dead. I only save and restore them if C2 uses the vector instructions (controlled by `SuperwordUseVSX`).
>> I have moved the non-volatile spill area out of the entry_frame_locals because it has a variable size, now.
>> 
>> The stack area for all non-volatile registers has become larger than the 288 Bytes which are allowed to be used below the SP (specified by the ABI). Therefore, I had to rewrite the call_stub sequence significantly. We need to push the new frame before saving the registers, now.
>> 
>> Saving and restoring the FP registers is not needed in the slow signature handler which also uses the save and restore code for non-volatile registers.
>> 
>> On Power10, we use vector pair instructions since Commit 8. E.g. in the call stub:
>> 
>>   0x000072c9483c07b4:   stxvp   vs52,-224(r2)
>>   0x000072c9483c07b8:   stxvp   vs54,-192(r2)
>>   0x000072c9483c07bc:   stxvp   vs56,-160(r2)
>>   0x000072c9483c07c0:   stxvp   vs58,-128(r2)
>>   0x000072c9483c07c4:   stxvp   vs60,-96(r2)
>>   0x000072c9483c07c8:   stxvp   vs62,-64(r2)
>> 
>> 
>> 
>>   0x000072c9483c0914:   lxvp    vs52,-224(r2)
>>   0x000072c9483c0918:   lxvp    vs54,-192(r2)
>>   0x000072c9483c091c:   lxvp    vs56,-160(r2)
>>   0x000072c9483c0920:   lxvp    vs58,-128(r2)
>>   0x000072c9483c0924:   lxvp    vs60,-96(r2)
>>   0x000072c9483c0928:   lxvp    vs62,-64(r2)
>
> Martin Doerr has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update Copyright header.

Hi Martin,
I do have a couple of questions and comments.
Still very nice!
Cheers, Richard.

src/hotspot/cpu/ppc/ppc.ad line 261:

> 259: // ----------------------------
> 260:   // 1st 32 VSRs are aliases for the FPRs which are already defined above.
> 261:   reg_def VSR0   (SOC, SOC, Op_VecX, 0, VMRegImpl::Bad());

I wonder how the old reg_defs worked, e.g. when allocating spill slots. Do you know?
BTW: You might use vector pair load/stores in `MachSpillCopyNode::implementation()` too.

src/hotspot/cpu/ppc/ppc.ad line 1934:

> 1932:   if (reg < 136+256) return rc_vs;
> 1933: 
> 1934:   assert(OptoReg::is_stack(reg), "what else is it?");

Maybe add
```c++
  assert(_last_Mach_Reg == 398, "hardcoded register indices need to be updated from enum MachRegisterNumbers");

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 141:

> 139:       __ rldicr(r_frame_size, r_frame_size, 3, 63 - 4);
> 140: 
> 141:       // this is the pure space for arguments

Suggestion:

      // this is the pure space for arguments (excluding alignment padding)

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 150:

> 148:       __ push_frame(r_frame_size, R0);
> 149: 
> 150:       // Save non-volatiles GPRs to ENTRY_FRAME (not yet pushed, but it's safe).

Frame's already pushed.
Suggestion:

      // Save non-volatiles registers to ENTRY_FRAME.

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 220:

> 218:       BLOCK_COMMENT("Call frame manager or native entry.");
> 219:       // Call frame manager or native entry.
> 220:       assert_different_registers(r_arg_entry, r_top_of_arguments_addr, r_arg_method, r_arg_thread);

Since you're at it: please adjust the 2 comment lines above: there's no frame manager. We're about to call the interpreter or native entry.
Also L222 (just remove "on entry ..."), L245. L265

src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 40:

> 38: static void preserve_callee_saved_registers(MacroAssembler* _masm, const ABIDescriptor& abi, int reg_save_area_offset) {
> 39:   __ block_comment("{ preserve_callee_saved_regs ");
> 40:   __ save_nonvolatile_registers(R1_SP, reg_save_area_offset, true, SuperwordUseVSX);

Parameter `abi` isn't used anymore. `preserve_callee_saved_registers` has just one call site. I think you should call `save_nonvolatile_registers` directly and delete this method. `block_comment` can be moved to `save_nonvolatile_registers`.

src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 46:

> 44: static void restore_callee_saved_registers(MacroAssembler* _masm, const ABIDescriptor& abi, int reg_save_area_offset) {
> 45:   __ block_comment("{ restore_callee_saved_regs ");
> 46:   __ restore_nonvolatile_registers(R1_SP, reg_save_area_offset, true, SuperwordUseVSX);

See comment on `preserve_callee_saved_registers`

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

PR Review: https://git.openjdk.org/jdk/pull/23987#pullrequestreview-2749247399
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2035094650
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2035073880
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2035684416
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2035685691
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2035807151
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2034626166
PR Review Comment: https://git.openjdk.org/jdk/pull/23987#discussion_r2034627304