Hi Haochen, Thanks for the update, some comments are inlined as below.
on 2022/12/2 15:03, HAO CHEN GUI wrote: > Hi, > For scalar extract/insert instructions, exponent field can be stored in a > 32-bit register. So this patch changes the mode of exponent field from DI to > SI so that these instructions can be generated in a 32-bit environment. Also > it removes TARGET_64BIT check for these instructions. > > The instructions using DI registers can be invoked with -mpowerpc64 in a > 32-bit environment. The patch changes insn condition from TARGET_64BIT to > TARGET_POWERPC64 for those instructions. > > This patch also changes prototypes and catagories of relevant built-ins and ~~~~~ categories > effective target checks of test cases. > > Compared to last version, main changes are to remove 64-bit environment > requirement for relevant built-ins in extend.texi. And to change the type of > arguments of relevant built-ins in rs6000-overload.def. > > Bootstrapped and tested on powerpc64-linux BE and LE with no regressions. > Is this okay for trunk? Any recommendations? Thanks a lot. > > ChangeLog > 2022-12-01 Haochen Gui <guih...@linux.ibm.com> > > gcc/ > * config/rs6000/rs6000-builtins.def > (__builtin_vsx_scalar_extract_exp): Set return type to const unsigned > int and move it from power9-64 to power9 catatlog. ~~~~~~~ catalog > (__builtin_vsx_scalar_extract_sig): Set return type to const unsigned > long long. > (__builtin_vsx_scalar_insert_exp): Set type of second argument to > unsigned int. > (__builtin_vsx_scalar_insert_exp_dp): Set type of second argument to > unsigned int and move it from power9-64 to power9 catatlog. ~~~~~~~ > * config/rs6000/vsx.md (xsxexpdp): Set mode of first operand to > SImode. Remove TARGET_64BIT from insn condition. > (xsxsigdp): Change insn condition from TARGET_64BIT to TARGET_POWERPC64. > (xsiexpdp): Change insn condition from TARGET_64BIT to > TARGET_POWERPC64. Set mode of third operand to SImode. > (xsiexpdpf): Set mode of third operand to SImode. Remove TARGET_64BIT > from insn condition. > * config/rs6000/rs6000-overload.def > (__builtin_vec_scalar_insert_exp): Set type of second argument to > unsigned int. > * doc/extend.texi (scalar_insert_exp): Set type of second argument to > unsigned int and remove 64-bit environment requirement when > significand has a float type. > (scalar_extract_exp): Remove 64-bit environment requirement. > > gcc/testsuite/ > * gcc.target/powerpc/bfp/scalar-extract-exp-0.c: Remove lp64 check. > * gcc.target/powerpc/bfp/scalar-extract-exp-1.c: Remove lp64 check. > * gcc.target/powerpc/bfp/scalar-extract-exp-2.c: Deleted as the case is > invalid now. > * gcc.target/powerpc/bfp/scalar-extract-exp-6.c: Replace lp64 check > with has_arch_ppc64. > * gcc.target/powerpc/bfp/scalar-extract-sig-0.c: Likewise. > * gcc.target/powerpc/bfp/scalar-extract-sig-6.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-0.c: Replace lp64 check > with has_arch_ppc64. Set type of exponent to unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-1.c: Set type of exponent > to unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-12.c: Replace lp64 check > with has_arch_ppc64. Set type of exponent to unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-13.c: Remove lp64 check. > Set type of exponent to unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-2.c: Set type of exponent to > unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-3.c: Remove lp64 check. Set > type of exponent to unsigned int. > * gcc.target/powerpc/bfp/scalar-insert-exp-4.c: Likewise. > * gcc.target/powerpc/bfp/scalar-insert-exp-5.c: Deleted as the case is > invalid now. > > patch.diff > diff --git a/gcc/config/rs6000/rs6000-builtins.def > b/gcc/config/rs6000/rs6000-builtins.def > index f76f54793d7..d8d67fa0cad 100644 > --- a/gcc/config/rs6000/rs6000-builtins.def > +++ b/gcc/config/rs6000/rs6000-builtins.def > @@ -2833,6 +2833,11 @@ > const signed int __builtin_dtstsfi_ov_td (const int<6>, _Decimal128); > TSTSFI_OV_TD dfptstsfi_unordered_td {} > > + const unsigned int __builtin_vsx_scalar_extract_exp (double); > + VSEEDP xsxexpdp {} > + > + const double __builtin_vsx_scalar_insert_exp_dp (double, unsigned int); > + VSIEDPF xsiexpdpf {} This __builtin_vsx_scalar_insert_exp_dp still requires 64-bit, see further explanation below ... > > [power9-64] > void __builtin_altivec_xst_len_r (vsc, void *, long); > @@ -2847,19 +2852,13 @@ > pure vsc __builtin_vsx_lxvl (const void *, signed long); > LXVL lxvl {} > > - const signed long __builtin_vsx_scalar_extract_exp (double); > - VSEEDP xsxexpdp {} > - > - const signed long __builtin_vsx_scalar_extract_sig (double); > + const unsigned long long __builtin_vsx_scalar_extract_sig (double); > VSESDP xsxsigdp {} > > const double __builtin_vsx_scalar_insert_exp (unsigned long long, \ > - unsigned long long); > + unsigned int); > VSIEDP xsiexpdp {} > > - const double __builtin_vsx_scalar_insert_exp_dp (double, unsigned long > long); > - VSIEDPF xsiexpdpf {} > - > pure vsc __builtin_vsx_xl_len_r (void *, signed long); > XL_LEN_R xl_len_r {} > > diff --git a/gcc/config/rs6000/rs6000-overload.def > b/gcc/config/rs6000/rs6000-overload.def > index 44e2945aaa0..8220d85271f 100644 > --- a/gcc/config/rs6000/rs6000-overload.def > +++ b/gcc/config/rs6000/rs6000-overload.def > @@ -4507,9 +4507,9 @@ > VSESQP > > [VEC_VSIE, scalar_insert_exp, __builtin_vec_scalar_insert_exp] > - double __builtin_vec_scalar_insert_exp (unsigned long long, unsigned long > long); > + double __builtin_vec_scalar_insert_exp (unsigned long long, unsigned int); > VSIEDP > - double __builtin_vec_scalar_insert_exp (double, unsigned long long); > + double __builtin_vec_scalar_insert_exp (double, unsigned int); > VSIEDPF > _Float128 __builtin_vec_scalar_insert_exp (unsigned __int128, unsigned > long long); > VSIEQP > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md > index e226a93bbe5..9d3a2340a79 100644 > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -5095,10 +5095,10 @@ (define_insn "xsxexpqp_<mode>" > > ;; VSX Scalar Extract Exponent Double-Precision > (define_insn "xsxexpdp" > - [(set (match_operand:DI 0 "register_operand" "=r") > - (unspec:DI [(match_operand:DF 1 "vsx_register_operand" "wa")] > + [(set (match_operand:SI 0 "register_operand" "=r") > + (unspec:SI [(match_operand:DF 1 "vsx_register_operand" "wa")] > UNSPEC_VSX_SXEXPDP))] > - "TARGET_P9_VECTOR && TARGET_64BIT" > + "TARGET_P9_VECTOR" > "xsxexpdp %0,%x1" > [(set_attr "type" "integer")]) > > @@ -5116,7 +5116,7 @@ (define_insn "xsxsigdp" > [(set (match_operand:DI 0 "register_operand" "=r") > (unspec:DI [(match_operand:DF 1 "vsx_register_operand" "wa")] > UNSPEC_VSX_SXSIG))] > - "TARGET_P9_VECTOR && TARGET_64BIT" > + "TARGET_P9_VECTOR && TARGET_POWERPC64" > "xsxsigdp %0,%x1" > [(set_attr "type" "integer")]) > > @@ -5145,9 +5145,9 @@ (define_insn "xsiexpqp_<mode>" > (define_insn "xsiexpdp" > [(set (match_operand:DF 0 "vsx_register_operand" "=wa") > (unspec:DF [(match_operand:DI 1 "register_operand" "r") > - (match_operand:DI 2 "register_operand" "r")] > + (match_operand:SI 2 "register_operand" "r")] > UNSPEC_VSX_SIEXPDP))] > - "TARGET_P9_VECTOR && TARGET_64BIT" > + "TARGET_P9_VECTOR && TARGET_POWERPC64" > "xsiexpdp %x0,%1,%2" > [(set_attr "type" "fpsimple")]) > > @@ -5155,9 +5155,9 @@ (define_insn "xsiexpdp" > (define_insn "xsiexpdpf" > [(set (match_operand:DF 0 "vsx_register_operand" "=wa") > (unspec:DF [(match_operand:DF 1 "register_operand" "r") > - (match_operand:DI 2 "register_operand" "r")] > + (match_operand:SI 2 "register_operand" "r")] > UNSPEC_VSX_SIEXPDP))] > - "TARGET_P9_VECTOR && TARGET_64BIT" > + "TARGET_P9_VECTOR" > "xsiexpdp %x0,%1,%2" > [(set_attr "type" "fpsimple")]) ... the final instruction xsiexpdp, as its description: src1 <- GPR[RA] // operand 1, src2 <- GPR[RB] VSR[32×TX+T].dword[0].bit[0] <- src1.bit[0] VSR[32×TX+T].dword[0].bit[1:11] <- src2.bit[53:63] VSR[32×TX+T].dword[0].bit[12:63] <- src1.bit[12:63] // it requires src1 should be 64 bits VSR[32×TX+T].dword[1] <- 0x0000_0000_0000_0000 Without TARGET_POWERPC64, we have to put the operand 1 into two separated GPRs, but only one GPR used by this instruction, the final result would be wrong. > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > index 7fe7f8817cd..cdadcfeb40e 100644 > --- a/gcc/doc/extend.texi > +++ b/gcc/doc/extend.texi > @@ -19552,8 +19552,8 @@ unsigned long long int scalar_extract_sig (double > source); > unsigned __int128 scalar_extract_sig (__ieee128 source); > > double scalar_insert_exp (unsigned long long int significand, > - unsigned long long int exponent); > -double scalar_insert_exp (double significand, unsigned long long int > exponent); > + unsigned int exponent); > +double scalar_insert_exp (double significand, unsigned int exponent); > > ieee_128 scalar_insert_exp (unsigned __int128 significand, > unsigned long long int exponent); > @@ -19573,7 +19573,9 @@ bool scalar_test_neg (double source); > bool scalar_test_neg (__ieee128 source); > @end smallexample > > -The @code{scalar_extract_exp} and @code{scalar_extract_sig} > +The @code{scalar_extract_exp} > +functions require an environment supporting ISA 3.0 or later. This change looks wrong, as the @code{scalar_extract_exp} functions in the context include both functions below: unsigned int scalar_extract_exp (double source); unsigned long long int scalar_extract_exp (__ieee128 source); The former doesn't requires 64-bit but the latter still requires (since IEEE128_HW requires 64-bit). So how about something like: "The @code{scalar_extract_exp} function with 64-bit source argument require an environment supporting ISA 3.0 or later. The @code{scalar_extract_exp} with 128-bit source argument and @code{scalar_extract_sig} functions require a 64-bit environment supporting ISA 3.0 or later. " This documentation update reminds me of that the current prototype of __ieee128 variant can be: unsigned int scalar_extract_exp (__ieee128 source); type unsigned int is enough for the exponent. It means xsxexpqp_<mode> can also use SImode rather than DImode. > +The @code{scalar_extract_sig} > functions require a 64-bit environment supporting ISA 3.0 or later. > The @code{scalar_extract_exp} and @code{scalar_extract_sig} built-in > functions return the significand and the biased exponent value > @@ -19591,8 +19593,10 @@ returned from the @code{scalar_extract_sig} > function. Use the > @code{scalar_test_neg} function to test the sign of its @code{double} > argument. > > -The @code{scalar_insert_exp} > +The @code{scalar_insert_exp} with an integer @code{significand} > functions require a 64-bit environment supporting ISA 3.0 or later. > +The @code{scalar_insert_exp} with a float @code{significand} > +functions require an environment supporting ISA 3.0 or later. > When supplied with a 64-bit first argument, the > @code{scalar_insert_exp} built-in function returns a double-precision > floating point value that is constructed by assembling the values of its > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-0.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-0.c > index 35bf1b240f3..a97a2b6ec98 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-0.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-0.c > @@ -1,9 +1,7 @@ > -/* { dg-do compile { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target powerpc_p9vector_ok } */ > +/* { dg-do compile } */ I noticed that some of updated test cases remove "{ target { powerpc*-*-* } } " from dg-do, but some don't (like closely following scalar-extract-exp-1.c). IMHO, we should keep consistent here, in this bucket we still have other scalar-* untouched, I prefer to keep it unchanged. Sorry that I didn't catch this in the previous review. > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target powerpc_p9vector_ok } */ > > -/* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> > > unsigned int > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-1.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-1.c > index 9737762c1d4..1cb438f9b70 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-1.c > @@ -1,9 +1,7 @@ > /* { dg-do compile { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > /* { dg-require-effective-target powerpc_p9vector_ok } */ > /* { dg-options "-mdejagnu-cpu=power8" } */ > > -/* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> > > unsigned int > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > deleted file mode 100644 > index 53b67c95cf9..00000000000 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-2.c > +++ /dev/null > @@ -1,20 +0,0 @@ > -/* { dg-do compile { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target ilp32 } */ > -/* { dg-require-effective-target powerpc_p9vector_ok } */ > -/* { dg-options "-mdejagnu-cpu=power9" } */ > - > -/* This test only runs on 32-bit configurations, where a compiler error > - should be issued because this builtin is not available on > - 32-bit configurations. */ > - > -#include <altivec.h> > - > -unsigned int > -get_exponent (double *p) > -{ > - double source = *p; > - > - return scalar_extract_exp (source); /* { dg-error > "'__builtin_vsx_scalar_extract_exp' requires the" } */ > -} > - > - > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-6.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-6.c > index b9dd7d61aae..136471a35b3 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-6.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-exp-6.c > @@ -1,7 +1,7 @@ > -/* { dg-do run { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target p9vector_hw } */ > +/* { dg-do run } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target has_arch_ppc64 } */ It gets rid of 64-bit env, why do we need has_arch_ppc64 here? ... > +/* { dg-require-effective-target p9vector_hw } */ > > /* This test should succeed only on 64-bit configurations. */ ... the comment is wrong if has_arch_ppc64 is not intentional. > #include <altivec.h> > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-0.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-0.c > index 637080652b7..3be7eb13566 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-0.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-0.c > @@ -1,7 +1,7 @@ > -/* { dg-do compile { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target powerpc_p9vector_ok } */ > +/* { dg-do compile } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target has_arch_ppc64 } */ > +/* { dg-require-effective-target powerpc_p9vector_ok } */ > > /* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> Missing to update scalar-extract-sig-1.c? It should use "has_arch_ppc64" instead. > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-6.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-6.c > index c85072da138..b96a745157d 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-6.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-extract-sig-6.c > @@ -1,7 +1,7 @@ > -/* { dg-do run { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target p9vector_hw } */ > +/* { dg-do run } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target has_arch_ppc64 } */ > +/* { dg-require-effective-target p9vector_hw } */ > > /* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-0.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-0.c > index d8243258a67..7e70d6e2642 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-0.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-0.c > @@ -1,17 +1,17 @@ > -/* { dg-do compile { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target powerpc_p9vector_ok } */ > +/* { dg-do compile } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target has_arch_ppc64 } */ > +/* { dg-require-effective-target powerpc_p9vector_ok } */ > > /* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> > > double > insert_exponent (unsigned long long int *significand_p, > - unsigned long long int *exponent_p) > + unsigned int *exponent_p) > { > unsigned long long int significand = *significand_p; > - unsigned long long int exponent = *exponent_p; > + unsigned int exponent = *exponent_p; > > return scalar_insert_exp (significand, exponent); > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-1.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-1.c > index 8260b107178..a7297319bc4 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-1.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-1.c > @@ -8,10 +8,10 @@ > > double > insert_exponent (unsigned long long int *significand_p, > - unsigned long long int *exponent_p) > + unsigned int *exponent_p) > { > unsigned long long int significand = *significand_p; > - unsigned long long int exponent = *exponent_p; > + unsigned int exponent = *exponent_p; > > return __builtin_vec_scalar_insert_exp (significand, exponent); /* { > dg-error "'__builtin_vsx_scalar_insert_exp' requires" } */ > } > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-12.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-12.c > index 384fc9cc675..343620fbe17 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-12.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-12.c > @@ -1,7 +1,7 @@ > -/* { dg-do run { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target p9vector_hw } */ > +/* { dg-do run } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target has_arch_ppc64 } */ > +/* { dg-require-effective-target p9vector_hw } */ > > /* This test should succeed only on 64-bit configurations. */ > #include <altivec.h> > @@ -9,10 +9,10 @@ > > double > insert_exponent (unsigned long long int *significand_p, > - unsigned long long int *exponent_p) > + unsigned int *exponent_p) > { > unsigned long long int significand = *significand_p; > - unsigned long long int exponent = *exponent_p; > + unsigned int exponent = *exponent_p; > > return scalar_insert_exp (significand, exponent); > } > @@ -24,8 +24,8 @@ main () > { > unsigned long long int significand_1 = 0x18000000000000LL; > unsigned long long int significand_2 = 0x1a000000000000LL; > - unsigned long long int exponent_1 = 62 + BIAS_FOR_DOUBLE_EXP; > - unsigned long long int exponent_2 = 49 + BIAS_FOR_DOUBLE_EXP; > + unsigned int exponent_1 = 62 + BIAS_FOR_DOUBLE_EXP; > + unsigned int exponent_2 = 49 + BIAS_FOR_DOUBLE_EXP; > > double x = (double) (0x1800ULL << 50); > double z = (double) (0x1a00ULL << 37); > diff --git a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-13.c > b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-13.c > index 0e004224277..6c3b1ccd523 100644 > --- a/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-13.c > +++ b/gcc/testsuite/gcc.target/powerpc/bfp/scalar-insert-exp-13.c > @@ -1,18 +1,16 @@ > -/* { dg-do run { target { powerpc*-*-* } } } */ > -/* { dg-require-effective-target lp64 } */ > -/* { dg-require-effective-target p9vector_hw } */ > +/* { dg-do run } */ > /* { dg-options "-mdejagnu-cpu=power9" } */ > +/* { dg-require-effective-target p9vector_hw } */ As the comment above, this scalar_insert_exp variant needs has_arch_ppc64, the others need to be updated accordingly. BR, Kewen