RFR: 8331861: [PPC64] Implement load / store assembler functions which take an Address object

[Martin Doerr]

On Mon, 14 Oct 2024 12:20:45 GMT, Varada M <varadam at openjdk.org> wrote:

> Load and store assembly instructions which takes Address object as argument. 
> 
> Tier 1 testing successful on linux-ppc64le and aix-ppc (fastdebug level)
> 
> JBS : [JDK-8331861](https://bugs.openjdk.org/browse/JDK-8331861)

Thanks! This looks better.

Please don't forget that an `Address` either has `_index` or `_disp` (but not both: https://github.com/openjdk/jdk/blob/f56a154132f7e66b1b65adfa2aa937119999b14a/src/hotspot/cpu/ppc/assembler_ppc.hpp#L44).

Idea: Use the `RegisterOrConstant` version. This covers all cases including large offset and index. E.g.
`inline void stw(  Register d, Address &a, Register tmp = noreg);`

inline void Assembler::stw(  Register d, Address &a, Register tmp) {
  stw(d, a.index() != noreg ? RegisterOrConstant(a.index()) : RegisterOrConstant(a.disp()), a.base(), tmp);
}

I'd move them into a separate section.

src/hotspot/cpu/ppc/assembler_ppc.hpp line 2534:

> 2532:   void stw( Register d, Address &a, Register tmp = noreg);
> 2533:   void sth( Register d, Address &a, Register tmp = noreg);
> 2534:   void stb( Register d, Address &a, Register tmp = noreg);

Spaces before "Register d" are uncommon. Please remove them (you can keep the one for ld to align it with the other ones).

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 341:

> 339: // PPC 1, section 3.3.2 Fixed-Point Load Instructions
> 340: inline void Assembler::lwzx( Register d, Register s1, Register s2) { emit_int32(LWZX_OPCODE | rt(d) | ra0mem(s1) | rb(s2));}
> 341: inline void Assembler::lwz(  Register d, Address &a) { lwz(d, a.disp(), a.base()); }

The load instructions need the same adaptation as the store instructions (just without the tmp Register).

inline void Assembler::lwz(  Register d, Address &a) {
  lwz(d, a.index() != noreg ? RegisterOrConstant(a.index()) : RegisterOrConstant(a.disp()), a.base());
}

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 380:

> 378: //  PPC 1, section 3.3.3 Fixed-Point Store Instructions
> 379: inline void Assembler::stwx( Register d, Register s1, Register s2) { emit_int32(STWX_OPCODE | rs(d) | ra0mem(s1) | rb(s2));}
> 380: inline void Assembler::stw(  Register d, Address &a, Register s1) { stw(d, a.index() != noreg ? RegisterOrConstant(a.index()) : RegisterOrConstant(a.disp()), a.base(), s1); }

I think the name "s1" is confusing, here. It's "tmp".

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 380:

> 378: //  PPC 1, section 3.3.3 Fixed-Point Store Instructions
> 379: inline void Assembler::stwx( Register d, Register s1, Register s2) { emit_int32(STWX_OPCODE | rs(d) | ra0mem(s1) | rb(s2));}
> 380: inline void Assembler::stw(  Register d, Address &a, Register tmp) { stw(d, a.index() != noreg ? RegisterOrConstant(a.index()) : RegisterOrConstant(a.disp()), a.base(), tmp); }

The line is very long. Better break it to:

inline void Assembler::stw(  Register d, Address &a, Register tmp) {
  stw(d, a.index() != noreg ? RegisterOrConstant(a.index()) : RegisterOrConstant(a.disp()), a.base(), tmp);
}

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

PR Review: https://git.openjdk.org/jdk/pull/21492#pullrequestreview-2392082045
PR Comment: https://git.openjdk.org/jdk/pull/21492#issuecomment-2411332924
PR Comment: https://git.openjdk.org/jdk/pull/21492#issuecomment-2414709306
PR Review Comment: https://git.openjdk.org/jdk/pull/21492#discussion_r1814734835
PR Review Comment: https://git.openjdk.org/jdk/pull/21492#discussion_r1814738595
PR Review Comment: https://git.openjdk.org/jdk/pull/21492#discussion_r1814714539
PR Review Comment: https://git.openjdk.org/jdk/pull/21492#discussion_r1814736276