On Thu, 22 Jan 2026 17:27:58 GMT, Andrew Haley <[email protected]> wrote:
>> Eric Fang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Extract some helper functions for better readability
>
> src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.hpp line 50:
>
>> 48: void sve_maxv(bool is_unsigned, FloatRegister dst, SIMD_RegVariant
>> size,
>> 49: PRegister pg, FloatRegister src);
>> 50:
>
> Using separate definitions here is adding unnecessary complexity.
>
> I'd do something like this in the header file:
>
>
> // Typedefs used to disambiguate overloaded member functions.
> typedef void (Assembler::*neon_reduction2)
> (FloatRegister, Assembler::SIMD_Arrangement, FloatRegister);
> typedef void (Assembler::*sve_reduction3)
> (FloatRegister, Assembler::SIMD_RegVariant, PRegister, FloatRegister);
>
> // Helper functions for min/max reduction operations
> void neon_minp(bool is_unsigned, FloatRegister dst, SIMD_Arrangement size,
> FloatRegister src1, FloatRegister src2) {
> auto m = is_unsigned ? &Assembler::uminp : &Assembler::sminp;
> (this->*m)(dst, size, src1, src2);
> }
> void neon_maxp(bool is_unsigned, FloatRegister dst, SIMD_Arrangement size,
> FloatRegister src1, FloatRegister src2) {
> auto m = is_unsigned ? &Assembler::umaxp : &Assembler::smaxp;
> (this->*m)(dst, size, src1, src2);
> }
> void neon_minv(bool is_unsigned, FloatRegister dst, SIMD_Arrangement size,
> FloatRegister src) {
> auto m = is_unsigned ? (neon_reduction2)&Assembler::uminv :
> &Assembler::sminv;
> (this->*m)(dst, size, src);
> }
> void neon_maxv(bool is_unsigned, FloatRegister dst, SIMD_Arrangement size,
> FloatRegister src) {
> auto m = is_unsigned ? (neon_reduction2)&Assembler::umaxv :
> &Assembler::smaxv;
> (this->*m)(dst, size, src);
> }
> void sve_minv(bool is_unsigned, FloatRegister dst, SIMD_RegVariant size,
> PRegister pg, FloatRegister src) {
> auto m = is_unsigned ? (sve_reduction3)&Assembler::sve_uminv :
> &Assembler::sve_sminv;
> (this->*m)(dst, size, pg, src);
> }
> void sve_maxv(bool is_unsigned, FloatRegister dst, SIMD_RegVariant size,
> PRegister pg, FloatRegister src) {
> auto m = is_unsigned ? (sve_reduction3)&Assembler::sve_umaxv :
> &Assembler::sve_smaxv;
> (this->*m)(dst, size, pg, src);
> }
>
>
>
> To some extent it's a matter of taste, but please try not to use much more
> repetitive and boilerplate code than you need to.
I’m not sure, but would the code below be better? It might make the macro
assembler a bit tidier.
void neon_minmaxp(bool is_unsigned, bool is_max, FloatRegister dst,
SIMD_Arrangement size, FloatRegister src1, FloatRegister
src2) {
auto m = is_unsigned ? (is_max ? &Assembler::umaxp : &Assembler::uminp)
: (is_max ? &Assembler::smaxp : &Assembler::sminp);
(this->*m)(dst, size, src1, src2);
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28693#discussion_r2753257260
