| Message ID | 20160718232509.GA31336@ibm-tiger.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 18, 2016 at 07:25:09PM -0400, Michael Meissner wrote: > When I added the support for __float128 last year, I accidentally broke > returning structures containing a single float or double item using the System > V 32-bit calling sequence. > > This patch goes back to using SCALAR_FLOAT_TYPE_P (which looks at the tree > node) instead of SCALAR_FLOAT_MODE_NOT_VECTOR_P (which only looks at the > mode). > > I have tested this patch on the trunk on a big endian power7 system, and there > were no regressions. The same patch applies to the GCC-6 branch, and I am > testing it now. Assuming there are no regresions on the GCC-6 branch, can I > check this patch into both the trunk and gcc-6-branch? Did you test with -m32, too? Ah the testcases (thanks) have it explicitly. Well. Does this work? > +/* { dg-do compile { target { powerpc*-*-linux* && ilp32 } } } */ > +/* { dg-options "-O2 -m32 -msvr4-struct-return" } */ Are dg-options set before the target test or after? If before, the ilp32 is superfluous; if after, the -m32 is. Or is there more to it? I think you can drop the ilp32. Please sort that out, make sure the testcases are actually run, and then this is okay for trunk as well as 6. Thanks for taking care of this! Segher
On Mon, Jul 18, 2016 at 06:42:02PM -0500, Segher Boessenkool wrote: > On Mon, Jul 18, 2016 at 07:25:09PM -0400, Michael Meissner wrote: > > When I added the support for __float128 last year, I accidentally broke > > returning structures containing a single float or double item using the System > > V 32-bit calling sequence. > > > > This patch goes back to using SCALAR_FLOAT_TYPE_P (which looks at the tree > > node) instead of SCALAR_FLOAT_MODE_NOT_VECTOR_P (which only looks at the > > mode). > > > > I have tested this patch on the trunk on a big endian power7 system, and there > > were no regressions. The same patch applies to the GCC-6 branch, and I am > > testing it now. Assuming there are no regresions on the GCC-6 branch, can I > > check this patch into both the trunk and gcc-6-branch? > > Did you test with -m32, too? > > Ah the testcases (thanks) have it explicitly. Well. Does this work? > > > +/* { dg-do compile { target { powerpc*-*-linux* && ilp32 } } } */ > > +/* { dg-options "-O2 -m32 -msvr4-struct-return" } */ Yes, both test out ok. > Are dg-options set before the target test or after? If before, the ilp32 > is superfluous; if after, the -m32 is. Or is there more to it? Not really, using ilp32 and explicit -m32 means -m32 is passed twice. I will remove the explicit -m32. > I think you can drop the ilp32. You cannot use -m32 on a 64-bit little endian system, so the && ilp32 test guarantees it is only run on a system that supports 32-bit (a pure 32-bit system, or a big endian 64-bit system that still has the 32-bit libraries installed). I also imagine somebody could build a 64-bit big endian compiler that was configured with --disable-multilib, and you would not be able to do -m32. > Please sort that out, make sure the testcases are actually run, and then > this is okay for trunk as well as 6. As I said, I dropped the explicit -m32 in dg-options. > Thanks for taking care of this!
On Mon, Jul 18, 2016 at 11:40:03PM -0400, Michael Meissner wrote: > > > +/* { dg-do compile { target { powerpc*-*-linux* && ilp32 } } } */ > > > +/* { dg-options "-O2 -m32 -msvr4-struct-return" } */ > > I think you can drop the ilp32. > > You cannot use -m32 on a 64-bit little endian system, so the && ilp32 test > guarantees it is only run on a system that supports 32-bit (a pure 32-bit > system, or a big endian 64-bit system that still has the 32-bit libraries > installed). ilp32 tests for a system that is *now* compiling for 32-bit, not one that *could*. > I also imagine somebody could build a 64-bit big endian compiler that was > configured with --disable-multilib, and you would not be able to do -m32. Ah, right, we would need a test saying "can compile for PowerPC 32-bit ELF" (this also includes powerpc-elf targets). There is no such test yet. What you do now works, okay. Thanks, Segher
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 238438) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -35467,7 +35467,8 @@ rs6000_function_value (const_tree valtyp if (DECIMAL_FLOAT_MODE_P (mode) && TARGET_HARD_FLOAT && TARGET_FPRS) /* _Decimal128 must use an even/odd register pair. */ regno = (mode == TDmode) ? FP_ARG_RETURN + 1 : FP_ARG_RETURN; - else if (SCALAR_FLOAT_MODE_NOT_VECTOR_P (mode) && TARGET_HARD_FLOAT && TARGET_FPRS + else if (SCALAR_FLOAT_TYPE_P (valtype) && TARGET_HARD_FLOAT && TARGET_FPRS + && !FLOAT128_VECTOR_P (mode) && ((TARGET_SINGLE_FLOAT && (mode == SFmode)) || TARGET_DOUBLE_FLOAT)) regno = FP_ARG_RETURN; else if (TREE_CODE (valtype) == COMPLEX_TYPE Index: gcc/testsuite/gcc.target/powerpc/pr71493-2.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr71493-2.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr71493-2.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-do compile { target { powerpc*-*-linux* && ilp32 } } } */ +/* { dg-options "-O2 -m32 -msvr4-struct-return" } */ + +struct S2 { double d; }; + +struct S2 foo2 (void) +{ + struct S2 s = { 1.0 }; + return s; +} + +/* { dg-final { scan-assembler "lwz" } } */ +/* { dg-final { scan-assembler-not "lfd" } } */ Index: gcc/testsuite/gcc.target/powerpc/pr71493-1.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/pr71493-1.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/pr71493-1.c (revision 0) @@ -0,0 +1,13 @@ +/* { dg-do compile { target { powerpc*-*-linux* && ilp32 } } } */ +/* { dg-options "-O2 -m32 -msvr4-struct-return" } */ + +struct S1 { float f; }; + +struct S1 foo1 (void) +{ + struct S1 s = { 1.0f }; + return s; +} + +/* { dg-final { scan-assembler "lwz" } } */ +/* { dg-final { scan-assembler-not "lfs" } } */