Message ID | 20110503212702.GA25985@hungry-tiger.westford.ibm.com |
---|---|
State | New |
Headers | show |
On Tue, May 3, 2011 at 5:27 PM, Michael Meissner <meissner@linux.vnet.ibm.com> wrote: > When I added VSX support to the powerpc, I overlooked passing and return > V2DImode arguments, since the only machine operation that supports V2DI is > vector floating point conversion. Consequentally, V2DI types were passed and > returned in GPRs instead of the vector registers on power7. > > This patch fixes that so that V2DImode values are passed and returned like > other vector types. > > I did a bootstrap and make check with no regressions, comparing it to a build > without the patch. I also wrote a program that passed and returned every > single type, and I compared the assembly ouptut. With the exception of > functions that return or are passed V2DI arguments, the code is identical. I > tested: > > -m64 (implies -mabi=altivec) > -m32 -mabi=altivec > -m32 -mabi=no-altivec (no difference here) > > Is this patch ok to install? I will also want to install it in the 4.6 and > possibly 4.5 trees as well. > > [gcc] > 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > PR target/48857 > * config/rs6000/rs6000.h (VSX_SCALAR_MODE): Delete. > (VSX_MODE): Ditto. > (VSX_MOVE_MODE): Ditto. > (ALTIVEC_OR_VSX_VECTOR_MODE): New macro, combine all Altivec and > VSX vector types. Add V2DImode. > (HARD_REGNO_CALLER_SAVE_MODE): Use it instead of > ALTIVEC_VECTOR_MODE and VSX_VECTOR_MODE calls. > (MODES_TIEABLE_P): Ditto. > > * config/rs6000/rs6000.c (rs6000_emit_move): Use > ALTIVEC_OR_VSX_MODE instead of ALTIVEC_VECTOR_MODE and > VSX_VECTOR_MODE. > (init_cumulative_args): Ditto. > (rs6000_function_arg_boundary): Ditto. > (rs6000_function_arg_advance_1): Ditto. > (rs6000_function_arg): Ditto. > (rs6000_function_ok_for_sibcall): Ditto. > (emit_frame_save): Ditto. > (rs6000_function_value): Ditto. > (rs6000_libcall_value): Ditto. > > [gcc/testsuite] > 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > PR target/48857 > * gcc.target/powerpc/pr48857.c: New file, make sure V2DI arguments > are passed and returned in vector registers. What does this do to the ABI? Haven't we now broken the ABI and broken backwards compatibility? - David
On Wed, May 4, 2011 at 4:51 AM, David Edelsohn <dje.gcc@gmail.com> wrote: > On Tue, May 3, 2011 at 5:27 PM, Michael Meissner > <meissner@linux.vnet.ibm.com> wrote: >> When I added VSX support to the powerpc, I overlooked passing and return >> V2DImode arguments, since the only machine operation that supports V2DI is >> vector floating point conversion. Consequentally, V2DI types were passed and >> returned in GPRs instead of the vector registers on power7. >> >> This patch fixes that so that V2DImode values are passed and returned like >> other vector types. >> >> I did a bootstrap and make check with no regressions, comparing it to a build >> without the patch. I also wrote a program that passed and returned every >> single type, and I compared the assembly ouptut. With the exception of >> functions that return or are passed V2DI arguments, the code is identical. I >> tested: >> >> -m64 (implies -mabi=altivec) >> -m32 -mabi=altivec >> -m32 -mabi=no-altivec (no difference here) >> >> Is this patch ok to install? I will also want to install it in the 4.6 and >> possibly 4.5 trees as well. >> >> [gcc] >> 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> >> >> PR target/48857 >> * config/rs6000/rs6000.h (VSX_SCALAR_MODE): Delete. >> (VSX_MODE): Ditto. >> (VSX_MOVE_MODE): Ditto. >> (ALTIVEC_OR_VSX_VECTOR_MODE): New macro, combine all Altivec and >> VSX vector types. Add V2DImode. >> (HARD_REGNO_CALLER_SAVE_MODE): Use it instead of >> ALTIVEC_VECTOR_MODE and VSX_VECTOR_MODE calls. >> (MODES_TIEABLE_P): Ditto. >> >> * config/rs6000/rs6000.c (rs6000_emit_move): Use >> ALTIVEC_OR_VSX_MODE instead of ALTIVEC_VECTOR_MODE and >> VSX_VECTOR_MODE. >> (init_cumulative_args): Ditto. >> (rs6000_function_arg_boundary): Ditto. >> (rs6000_function_arg_advance_1): Ditto. >> (rs6000_function_arg): Ditto. >> (rs6000_function_ok_for_sibcall): Ditto. >> (emit_frame_save): Ditto. >> (rs6000_function_value): Ditto. >> (rs6000_libcall_value): Ditto. >> >> [gcc/testsuite] >> 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> >> >> PR target/48857 >> * gcc.target/powerpc/pr48857.c: New file, make sure V2DI arguments >> are passed and returned in vector registers. > > What does this do to the ABI? Haven't we now broken the ABI and > broken backwards compatibility? It at least looks like so. You need to add appropriate changes.html entries to all branches you apply this patch to. I suppose the new version matches what XLC does? Richard. > - David >
On Wed, 2011-05-04 at 11:33 +0200, Richard Guenther wrote: > On Wed, May 4, 2011 at 4:51 AM, David Edelsohn <dje.gcc@gmail.com> wrote: > > On Tue, May 3, 2011 at 5:27 PM, Michael Meissner <meissner@linux.vnet.ibm.com> wrote: > >> > >> This patch fixes that so that V2DImode values are passed and returned like > >> other vector types. [snip] > > What does this do to the ABI? Haven't we now broken the ABI and > > broken backwards compatibility? > > It at least looks like so. You need to add appropriate changes.html > entries to all branches you apply this patch to. I suppose the new > version matches what XLC does? Yes, XLC passes V2DI parameters and return values in Altivec registers as per the ABI, so we're the buggy ones. Peter
On Tue, May 03, 2011 at 10:51:51PM -0400, David Edelsohn wrote: > What does this do to the ABI? Haven't we now broken the ABI and > broken backwards compatibility? In this case, GCC wasn't following the ABI. My patch will have GCC follow the ABI. The XL compiler also follows the ABI. So we have to decide whether to keep the current calling sequence which violates the ABI, or change the compiler to follow the ABI, and break backwards compatiblity. Given the machine doesn't have native VD2Imode operations, except for conversion to/from floating point, and the bitwise operations, I suspect not to many people use the data type. Note, V2DImode was added in GCC 4.5 when the VSX support was added. It was not in the earlier Altivec support. The MODES_TIEABLE_P part of the patch will need to go in to fix PR 48495 (it was in looking at the bug we discovered that the compiler was not following the ABI).
On Wed, May 04, 2011 at 11:33:43AM +0200, Richard Guenther wrote: > On Wed, May 4, 2011 at 4:51 AM, David Edelsohn <dje.gcc@gmail.com> wrote: > > What does this do to the ABI? Haven't we now broken the ABI and > > broken backwards compatibility? > > It at least looks like so. You need to add appropriate changes.html > entries to all branches you apply this patch to. I suppose the new > version matches what XLC does? Yes. I will do that if the patch is approved. As I said in my other reply, the patch will make us compatible with XLC.
On Tue, May 3, 2011 at 5:27 PM, Michael Meissner <meissner@linux.vnet.ibm.com> wrote: > When I added VSX support to the powerpc, I overlooked passing and return > V2DImode arguments, since the only machine operation that supports V2DI is > vector floating point conversion. Consequentally, V2DI types were passed and > returned in GPRs instead of the vector registers on power7. > > This patch fixes that so that V2DImode values are passed and returned like > other vector types. > > I did a bootstrap and make check with no regressions, comparing it to a build > without the patch. I also wrote a program that passed and returned every > single type, and I compared the assembly ouptut. With the exception of > functions that return or are passed V2DI arguments, the code is identical. I > tested: > > -m64 (implies -mabi=altivec) > -m32 -mabi=altivec > -m32 -mabi=no-altivec (no difference here) > > Is this patch ok to install? I will also want to install it in the 4.6 and > possibly 4.5 trees as well. > > [gcc] > 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > PR target/48857 > * config/rs6000/rs6000.h (VSX_SCALAR_MODE): Delete. > (VSX_MODE): Ditto. > (VSX_MOVE_MODE): Ditto. > (ALTIVEC_OR_VSX_VECTOR_MODE): New macro, combine all Altivec and > VSX vector types. Add V2DImode. > (HARD_REGNO_CALLER_SAVE_MODE): Use it instead of > ALTIVEC_VECTOR_MODE and VSX_VECTOR_MODE calls. > (MODES_TIEABLE_P): Ditto. > > * config/rs6000/rs6000.c (rs6000_emit_move): Use > ALTIVEC_OR_VSX_MODE instead of ALTIVEC_VECTOR_MODE and > VSX_VECTOR_MODE. > (init_cumulative_args): Ditto. > (rs6000_function_arg_boundary): Ditto. > (rs6000_function_arg_advance_1): Ditto. > (rs6000_function_arg): Ditto. > (rs6000_function_ok_for_sibcall): Ditto. > (emit_frame_save): Ditto. > (rs6000_function_value): Ditto. > (rs6000_libcall_value): Ditto. > > [gcc/testsuite] > 2011-05-03 Michael Meissner <meissner@linux.vnet.ibm.com> > > PR target/48857 > * gcc.target/powerpc/pr48857.c: New file, make sure V2DI arguments > are passed and returned in vector registers. The patch is okay, but please add a comment explaining why the function_value and libcall_value changes are correct. Thanks, David
Index: gcc/config/rs6000/rs6000.h =================================================================== --- gcc/config/rs6000/rs6000.h (revision 173266) +++ gcc/config/rs6000/rs6000.h (working copy) @@ -1007,10 +1007,9 @@ extern unsigned rs6000_pointer_size; /* When setting up caller-save slots (MODE == VOIDmode) ensure we allocate enough space to account for vectors in FP regs. */ -#define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE) \ - (TARGET_VSX \ - && ((MODE) == VOIDmode || VSX_VECTOR_MODE (MODE) \ - || ALTIVEC_VECTOR_MODE (MODE)) \ +#define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE) \ + (TARGET_VSX \ + && ((MODE) == VOIDmode || ALTIVEC_OR_VSX_VECTOR_MODE (MODE)) \ && FP_REGNO_P (REGNO) \ ? V2DFmode \ : choose_hard_reg_mode ((REGNO), (NREGS), false)) @@ -1026,25 +1025,16 @@ extern unsigned rs6000_pointer_size; ((MODE) == V4SFmode \ || (MODE) == V2DFmode) \ -#define VSX_SCALAR_MODE(MODE) \ - ((MODE) == DFmode) - -#define VSX_MODE(MODE) \ - (VSX_VECTOR_MODE (MODE) \ - || VSX_SCALAR_MODE (MODE)) - -#define VSX_MOVE_MODE(MODE) \ - (VSX_VECTOR_MODE (MODE) \ - || VSX_SCALAR_MODE (MODE) \ - || ALTIVEC_VECTOR_MODE (MODE) \ - || (MODE) == TImode) - #define ALTIVEC_VECTOR_MODE(MODE) \ ((MODE) == V16QImode \ || (MODE) == V8HImode \ || (MODE) == V4SFmode \ || (MODE) == V4SImode) +#define ALTIVEC_OR_VSX_VECTOR_MODE(MODE) \ + (ALTIVEC_VECTOR_MODE (MODE) || VSX_VECTOR_MODE (MODE) \ + || (MODE) == V2DImode) + #define SPE_VECTOR_MODE(MODE) \ ((MODE) == V4HImode \ || (MODE) == V2SFmode \ @@ -1080,10 +1070,10 @@ extern unsigned rs6000_pointer_size; ? ALTIVEC_VECTOR_MODE (MODE2) \ : ALTIVEC_VECTOR_MODE (MODE2) \ ? ALTIVEC_VECTOR_MODE (MODE1) \ - : VSX_VECTOR_MODE (MODE1) \ - ? VSX_VECTOR_MODE (MODE2) \ - : VSX_VECTOR_MODE (MODE2) \ - ? VSX_VECTOR_MODE (MODE1) \ + : ALTIVEC_OR_VSX_VECTOR_MODE (MODE1) \ + ? ALTIVEC_OR_VSX_VECTOR_MODE (MODE2) \ + : ALTIVEC_OR_VSX_VECTOR_MODE (MODE2) \ + ? ALTIVEC_OR_VSX_VECTOR_MODE (MODE1) \ : 1) /* Post-reload, we can't use any new AltiVec registers, as we already Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 173266) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -7902,7 +7902,7 @@ rs6000_emit_move (rtx dest, rtx source, /* Nonzero if we can use an AltiVec register to pass this arg. */ #define USE_ALTIVEC_FOR_ARG_P(CUM,MODE,TYPE,NAMED) \ - ((ALTIVEC_VECTOR_MODE (MODE) || VSX_VECTOR_MODE (MODE)) \ + (ALTIVEC_OR_VSX_VECTOR_MODE (MODE) \ && (CUM)->vregno <= ALTIVEC_ARG_MAX_REG \ && TARGET_ALTIVEC_ABI \ && (NAMED)) @@ -8102,8 +8102,7 @@ init_cumulative_args (CUMULATIVE_ARGS *c } if (SCALAR_FLOAT_MODE_P (return_mode)) rs6000_passes_float = true; - else if (ALTIVEC_VECTOR_MODE (return_mode) - || VSX_VECTOR_MODE (return_mode) + else if (ALTIVEC_OR_VSX_VECTOR_MODE (return_mode) || SPE_VECTOR_MODE (return_mode)) rs6000_passes_vector = true; } @@ -8218,7 +8217,7 @@ rs6000_function_arg_boundary (enum machi && int_size_in_bytes (type) >= 8 && int_size_in_bytes (type) < 16)) return 64; - else if ((ALTIVEC_VECTOR_MODE (mode) || VSX_VECTOR_MODE (mode)) + else if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || (type && TREE_CODE (type) == VECTOR_TYPE && int_size_in_bytes (type) >= 16)) return 128; @@ -8438,7 +8437,7 @@ rs6000_function_arg_advance_1 (CUMULATIV { if (SCALAR_FLOAT_MODE_P (mode)) rs6000_passes_float = true; - else if (named && (ALTIVEC_VECTOR_MODE (mode) || VSX_VECTOR_MODE (mode))) + else if (named && ALTIVEC_OR_VSX_VECTOR_MODE (mode)) rs6000_passes_vector = true; else if (SPE_VECTOR_MODE (mode) && !cum->stdarg @@ -8448,8 +8447,7 @@ rs6000_function_arg_advance_1 (CUMULATIV #endif if (TARGET_ALTIVEC_ABI - && (ALTIVEC_VECTOR_MODE (mode) - || VSX_VECTOR_MODE (mode) + && (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || (type && TREE_CODE (type) == VECTOR_TYPE && int_size_in_bytes (type) == 16))) { @@ -9067,8 +9065,7 @@ rs6000_function_arg (CUMULATIVE_ARGS *cu else return gen_rtx_REG (mode, cum->vregno); else if (TARGET_ALTIVEC_ABI - && (ALTIVEC_VECTOR_MODE (mode) - || VSX_VECTOR_MODE (mode) + && (ALTIVEC_OR_VSX_VECTOR_MODE (mode) || (type && TREE_CODE (type) == VECTOR_TYPE && int_size_in_bytes (type) == 16))) { @@ -19355,14 +19352,12 @@ rs6000_function_ok_for_sibcall (tree dec here. */ FOREACH_FUNCTION_ARGS(fntype, type, args_iter) if (TREE_CODE (type) == VECTOR_TYPE - && (ALTIVEC_VECTOR_MODE (TYPE_MODE (type)) - || VSX_VECTOR_MODE (TYPE_MODE (type)))) + && ALTIVEC_OR_VSX_VECTOR_MODE (TYPE_MODE (type))) nvreg++; FOREACH_FUNCTION_ARGS(TREE_TYPE (current_function_decl), type, args_iter) if (TREE_CODE (type) == VECTOR_TYPE - && (ALTIVEC_VECTOR_MODE (TYPE_MODE (type)) - || VSX_VECTOR_MODE (TYPE_MODE (type)))) + && ALTIVEC_OR_VSX_VECTOR_MODE (TYPE_MODE (type))) nvreg--; if (nvreg > 0) @@ -20075,7 +20070,7 @@ emit_frame_save (rtx frame_reg, rtx fram /* Some cases that need register indexed addressing. */ if ((TARGET_ALTIVEC_ABI && ALTIVEC_VECTOR_MODE (mode)) - || (TARGET_VSX && VSX_VECTOR_MODE (mode)) + || (TARGET_VSX && ALTIVEC_OR_VSX_VECTOR_MODE (mode)) || (TARGET_E500_DOUBLE && mode == DFmode) || (TARGET_SPE_ABI && SPE_VECTOR_MODE (mode) @@ -27361,11 +27356,7 @@ rs6000_function_value (const_tree valtyp return rs6000_complex_function_value (mode); else if (TREE_CODE (valtype) == VECTOR_TYPE && TARGET_ALTIVEC && TARGET_ALTIVEC_ABI - && ALTIVEC_VECTOR_MODE (mode)) - regno = ALTIVEC_ARG_RETURN; - else if (TREE_CODE (valtype) == VECTOR_TYPE - && TARGET_VSX && TARGET_ALTIVEC_ABI - && VSX_VECTOR_MODE (mode)) + && ALTIVEC_OR_VSX_VECTOR_MODE (mode)) regno = ALTIVEC_ARG_RETURN; else if (TARGET_E500_DOUBLE && TARGET_HARD_FLOAT && (mode == DFmode || mode == DCmode @@ -27405,12 +27396,9 @@ rs6000_libcall_value (enum machine_mode && TARGET_HARD_FLOAT && TARGET_FPRS && ((TARGET_SINGLE_FLOAT && mode == SFmode) || TARGET_DOUBLE_FLOAT)) regno = FP_ARG_RETURN; - else if (ALTIVEC_VECTOR_MODE (mode) + else if (ALTIVEC_OR_VSX_VECTOR_MODE (mode) && TARGET_ALTIVEC && TARGET_ALTIVEC_ABI) regno = ALTIVEC_ARG_RETURN; - else if (VSX_VECTOR_MODE (mode) - && TARGET_VSX && TARGET_ALTIVEC_ABI) - regno = ALTIVEC_ARG_RETURN; else if (COMPLEX_MODE_P (mode) && targetm.calls.split_complex_arg) return rs6000_complex_function_value (mode); else if (TARGET_E500_DOUBLE && TARGET_HARD_FLOAT Index: gcc/testsuite/gcc.target/powerpc/pr48857.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr48857.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr48857.c (revision 0) @@ -0,0 +1,25 @@ +/* { dg-do compile { target { powerpc*-*-* } } } */ +/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */ +/* { dg-require-effective-target powerpc_vsx_ok } */ +/* { dg-options "-O2 -mcpu=power7 -mabi=altivec" } */ +/* { dg-final { scan-assembler-times "lxvd2x" 1 } } */ +/* { dg-final { scan-assembler-times "stxvd2x" 1 } } */ +/* { dg-final { scan-assembler-not "ld" } } */ +/* { dg-final { scan-assembler-not "lwz" } } */ +/* { dg-final { scan-assembler-not "stw" } } */ +/* { dg-final { scan-assembler-not "addi" } } */ + +typedef vector long long v2di_type; + +v2di_type +return_v2di (v2di_type *ptr) +{ + return *ptr; /* should generate lxvd2x 34,0,3. */ +} + +void +pass_v2di (v2di_type arg, v2di_type *ptr) +{ + *ptr = arg; /* should generate stxvd2x 34,0,{3,5}. */ +} +