| Message ID | cfbb608f-43e1-d6d5-9ad7-4a764ecd9146@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Hi! On Thu, May 04, 2017 at 04:35:10PM -0500, Bill Schmidt wrote: > In an earlier patch, I changed vec_xl and vec_xst to make use of new > POWER9 instructions when loading or storing vector short/char values. > In so doing, I failed to enable the existing instruction use for > -mcpu=power8, so these were no longer considered valid by the compiler. > Not good. > > This patch fixes the problem by using other existing built-in definitions > when the POWER9 instructions are not available. I've added a test case > to improve coverage and demonstrate that the problem is fixed. > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > regressions. Is this ok for trunk? Yes, thanks! One nit: > --- gcc/config/rs6000/rs6000.c (revision 247560) > +++ gcc/config/rs6000/rs6000.c (working copy) > @@ -18183,6 +18183,17 @@ altivec_init_builtins (void) > def_builtin ("__builtin_vsx_st_elemrev_v16qi", > void_ftype_v16qi_long_pvoid, VSX_BUILTIN_ST_ELEMREV_V16QI); > } > + else > + { > + rs6000_builtin_decls[(int)VSX_BUILTIN_LD_ELEMREV_V8HI] > + = rs6000_builtin_decls[(int)VSX_BUILTIN_LXVW4X_V8HI]; There should be a space after the cast operators. Segher
On May 9, 2017, at 9:58 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Thu, May 04, 2017 at 04:35:10PM -0500, Bill Schmidt wrote: >> In an earlier patch, I changed vec_xl and vec_xst to make use of new >> POWER9 instructions when loading or storing vector short/char values. >> In so doing, I failed to enable the existing instruction use for >> -mcpu=power8, so these were no longer considered valid by the compiler. >> Not good. >> >> This patch fixes the problem by using other existing built-in definitions >> when the POWER9 instructions are not available. I've added a test case >> to improve coverage and demonstrate that the problem is fixed. >> >> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no >> regressions. Is this ok for trunk? > > Yes, thanks! One nit: > >> --- gcc/config/rs6000/rs6000.c (revision 247560) >> +++ gcc/config/rs6000/rs6000.c (working copy) >> @@ -18183,6 +18183,17 @@ altivec_init_builtins (void) >> def_builtin ("__builtin_vsx_st_elemrev_v16qi", >> void_ftype_v16qi_long_pvoid, VSX_BUILTIN_ST_ELEMREV_V16QI); >> } >> + else >> + { >> + rs6000_builtin_decls[(int)VSX_BUILTIN_LD_ELEMREV_V8HI] >> + = rs6000_builtin_decls[(int)VSX_BUILTIN_LXVW4X_V8HI]; > > There should be a space after the cast operators. OK, will fix. Thanks for the review! I forgot to ask -- this fix is needed for GCC 6 and 7 as well. Is this ok for backport after the usual burn-in? Thanks, Bill > > > Segher >
On Tue, May 09, 2017 at 01:44:35PM -0500, Bill Schmidt wrote: > I forgot to ask -- this fix is needed for GCC 6 and 7 as well. Is this ok for backport > after the usual burn-in? Sure! Segher
> On May 9, 2017, at 5:28 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Tue, May 09, 2017 at 01:44:35PM -0500, Bill Schmidt wrote: >> I forgot to ask -- this fix is needed for GCC 6 and 7 as well. Is this ok for backport >> after the usual burn-in? > > Sure! > Thanks! GCC 7: r247999; GCC 8: r248005. Bill > > Segher >
On May 12, 2017, at 9:17 PM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote: > > >> On May 9, 2017, at 5:28 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> >> On Tue, May 09, 2017 at 01:44:35PM -0500, Bill Schmidt wrote: >>> I forgot to ask -- this fix is needed for GCC 6 and 7 as well. Is this ok for backport >>> after the usual burn-in? >> >> Sure! >> > Thanks! GCC 7: r247999; GCC 8: r248005. Grumble, that last should read GCC 6: r248005. Bill > > Bill >> >> Segher >> >
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 247560) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -18183,6 +18183,17 @@ altivec_init_builtins (void) def_builtin ("__builtin_vsx_st_elemrev_v16qi", void_ftype_v16qi_long_pvoid, VSX_BUILTIN_ST_ELEMREV_V16QI); } + else + { + rs6000_builtin_decls[(int)VSX_BUILTIN_LD_ELEMREV_V8HI] + = rs6000_builtin_decls[(int)VSX_BUILTIN_LXVW4X_V8HI]; + rs6000_builtin_decls[(int)VSX_BUILTIN_LD_ELEMREV_V16QI] + = rs6000_builtin_decls[(int)VSX_BUILTIN_LXVW4X_V16QI]; + rs6000_builtin_decls[(int)VSX_BUILTIN_ST_ELEMREV_V8HI] + = rs6000_builtin_decls[(int)VSX_BUILTIN_STXVW4X_V8HI]; + rs6000_builtin_decls[(int)VSX_BUILTIN_ST_ELEMREV_V16QI] + = rs6000_builtin_decls[(int)VSX_BUILTIN_STXVW4X_V16QI]; + } def_builtin ("__builtin_vec_vsx_ld", opaque_ftype_long_pcvoid, VSX_BUILTIN_VEC_LD); Index: gcc/testsuite/gcc.target/powerpc/p8-vec-xl-xst.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/p8-vec-xl-xst.c (nonexistent) +++ gcc/testsuite/gcc.target/powerpc/p8-vec-xl-xst.c (working copy) @@ -0,0 +1,62 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */ +/* { dg-require-effective-target powerpc_p8vector_ok } */ +/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */ +/* { dg-options "-mcpu=power8 -O2" } */ + +/* Verify fix for problem where vec_xl and vec_xst are not recognized + for the vector char and vector short cases on P8 only. */ + +#include <altivec.h> + +vector unsigned char +foo (unsigned char * address) +{ + return __builtin_vec_xl (0, address); +} + +void +bar (vector unsigned char x, unsigned char * address) +{ + __builtin_vec_xst (x, 0, address); +} + +vector unsigned short +foot (unsigned short * address) +{ + return __builtin_vec_xl (0, address); +} + +void +bart (vector unsigned short x, unsigned short * address) +{ + __builtin_vec_xst (x, 0, address); +} + +vector unsigned char +fool (unsigned char * address) +{ + return vec_xl (0, address); +} + +void +barl (vector unsigned char x, unsigned char * address) +{ + vec_xst (x, 0, address); +} + +vector unsigned short +footle (unsigned short * address) +{ + return vec_xl (0, address); +} + +void +bartle (vector unsigned short x, unsigned short * address) +{ + vec_xst (x, 0, address); +} + +/* { dg-final { scan-assembler-times "lxvd2x" 4 } } */ +/* { dg-final { scan-assembler-times "stxvd2x" 4 } } */ +/* { dg-final { scan-assembler-times "xxpermdi" 8 } } */