public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
@ 2017-04-04 19:24 Jakub Jelinek
  2017-04-05  7:42 ` Uros Bizjak
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2017-04-04 19:24 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches

Hi!

aggregate_value_p is called often very early during compilation, e.g.
from allocate_function or during gimplification of a call with lhs.
The problem with that is e.g. that on x86_64 -m64 -mno-sse we can't
include <x86intrin.h>, because the always_inline inline functions
in mmx and 3dnow intrinsic headers return __m64 or take __m64 as arguments
and that in the 64-bit ABI is in SSE register.

The following patch makes sure we diagnose this only later (e.g. when
expanding a function to RTL or when expanding calls to other functions),
which means we don't diagnose e.g. inline functions that got successfully
inlined (because then there is really no function return in SSE or x87
reg) or e.g. for builtin calls if they are emitted inline rather than
as a library call (again, I think that is desirable).
I had to tweak a few tests because the reported line changed slightly,
and in the last test add -fno-builtin-fminl, because otherwise fminl
is expanded inline and again there is no call left with the problem.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR target/80298
	* config/i386/i386.c (construct_container): Postpone errors about
	return values or arguments in SSE or x87 registers with those
	disabled until inlining is done.

	* gcc.target/i386/pr80298-1.c: New test.
	* gcc.target/i386/pr80298-2.c: New test.
	* gcc.target/i386/pr57655.c: Adjust expected diagnostic line.
	* gcc.target/i386/pr59794-6.c: Likewise.
	* gcc.target/i386/pr70738-1.c: Likewise.  Add -Wno-psabi to
	dg-options.
	* gcc.target/i386/pr68473-1.c: Add -fno-builtin-fminl to dg-options.

--- gcc/config/i386/i386.c.jj	2017-04-04 19:51:33.661684106 +0200
+++ gcc/config/i386/i386.c	2017-04-04 20:45:42.573366752 +0200
@@ -9330,7 +9330,12 @@ construct_container (machine_mode mode,
      some less clueful developer tries to use floating-point anyway.  */
   if (needed_sseregs && !TARGET_SSE)
     {
-      if (in_return)
+      /* Don't diagnose anything until after inlining, we might have
+	 functions with such arguments that are just always inlined
+	 and don't really need SSE returns or arguments.  */
+      if (symtab->state < IPA_SSA_AFTER_INLINING)
+        ;
+      else if (in_return)
 	{
 	  if (!issued_sse_ret_error)
 	    {
@@ -9354,7 +9359,11 @@ construct_container (machine_mode mode,
 	  || regclass[i] == X86_64_X87UP_CLASS
 	  || regclass[i] == X86_64_COMPLEX_X87_CLASS)
 	{
-	  if (!issued_x87_ret_error)
+	  /* Don't diagnose anything until after inlining, we might have
+	     functions with such arguments that are just always inlined
+	     and don't really need x87 returns.  */
+	  if (symtab->state >= IPA_SSA_AFTER_INLINING
+	      && !issued_x87_ret_error)
 	    {
 	      error ("x87 register return with x87 disabled");
 	      issued_x87_ret_error = true;
--- gcc/testsuite/gcc.target/i386/pr80298-1.c.jj	2017-04-04 20:45:42.574366739 +0200
+++ gcc/testsuite/gcc.target/i386/pr80298-1.c	2017-04-04 20:45:42.574366739 +0200
@@ -0,0 +1,7 @@
+/* PR target/80298 */
+/* { dg-do compile } */
+/* { dg-options "-mno-sse -mmmx" } */
+
+#include <x86intrin.h>
+
+int i;
--- gcc/testsuite/gcc.target/i386/pr80298-2.c.jj	2017-04-04 20:45:42.574366739 +0200
+++ gcc/testsuite/gcc.target/i386/pr80298-2.c	2017-04-04 20:45:42.574366739 +0200
@@ -0,0 +1,7 @@
+/* PR target/80298 */
+/* { dg-do compile } */
+/* { dg-options "-mno-sse -mmmx -O2" } */
+
+#include <x86intrin.h>
+
+int i;
--- gcc/testsuite/gcc.target/i386/pr57655.c.jj	2016-05-22 12:20:31.000000000 +0200
+++ gcc/testsuite/gcc.target/i386/pr57655.c	2017-04-04 20:48:31.867154730 +0200
@@ -1,7 +1,7 @@
 /* { dg-do compile } */
 /* { dg-options "-mavx -mvzeroupper -mno-fp-ret-in-387" }
 
-/* { dg-error "x87 register return with x87 disabled" "" { target { ! ia32 } } 8 } */
+/* { dg-error "x87 register return with x87 disabled" "" { target { ! ia32 } } 7 } */
 
 long double
 foo (long double x)
--- gcc/testsuite/gcc.target/i386/pr59794-6.c.jj	2014-01-15 08:11:25.000000000 +0100
+++ gcc/testsuite/gcc.target/i386/pr59794-6.c	2017-04-04 20:49:21.820502031 +0200
@@ -8,7 +8,7 @@ typedef int __v4si __attribute__ ((__vec
 extern __v4si x;
 
 __v4si
-foo (void)
-{ /* { dg-error "SSE register return with SSE disabled" } */
+foo (void) /* { dg-error "SSE register return with SSE disabled" } */
+{
   return x;
 }
--- gcc/testsuite/gcc.target/i386/pr70738-1.c.jj	2016-05-26 10:37:56.000000000 +0200
+++ gcc/testsuite/gcc.target/i386/pr70738-1.c	2017-04-04 20:54:32.750439367 +0200
@@ -1,9 +1,9 @@
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-msse2 -mgeneral-regs-only" } */
+/* { dg-options "-msse2 -mgeneral-regs-only -Wno-psabi" } */
 
 typedef int int32x2_t __attribute__ ((__vector_size__ ((8))));
 
-int32x2_t test (int32x2_t a, int32x2_t b)
-{ /* { dg-error "SSE register return with SSE disabled" } */
+int32x2_t test (int32x2_t a, int32x2_t b) /* { dg-error "SSE register return with SSE disabled" } */
+{
   return a + b;
 }
--- gcc/testsuite/gcc.target/i386/pr68473-1.c.jj	2015-12-31 01:11:11.000000000 +0100
+++ gcc/testsuite/gcc.target/i386/pr68473-1.c	2017-04-04 21:08:26.668514992 +0200
@@ -1,5 +1,5 @@
 /* { dg-do compile { target { ! ia32 } } } */
-/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387" } */
+/* { dg-options "-fdiagnostics-show-caret -mno-fp-ret-in-387 -fno-builtin-fminl" } */
 
 extern long double fminl (long double __x, long double __y);
 

	Jakub

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-04 19:24 [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298) Jakub Jelinek
@ 2017-04-05  7:42 ` Uros Bizjak
  2017-04-05  8:00   ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Uros Bizjak @ 2017-04-05  7:42 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Tue, Apr 4, 2017 at 9:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> aggregate_value_p is called often very early during compilation, e.g.
> from allocate_function or during gimplification of a call with lhs.
> The problem with that is e.g. that on x86_64 -m64 -mno-sse we can't
> include <x86intrin.h>, because the always_inline inline functions
> in mmx and 3dnow intrinsic headers return __m64 or take __m64 as arguments
> and that in the 64-bit ABI is in SSE register.
>
> The following patch makes sure we diagnose this only later (e.g. when
> expanding a function to RTL or when expanding calls to other functions),
> which means we don't diagnose e.g. inline functions that got successfully
> inlined (because then there is really no function return in SSE or x87
> reg) or e.g. for builtin calls if they are emitted inline rather than
> as a library call (again, I think that is desirable).
> I had to tweak a few tests because the reported line changed slightly,
> and in the last test add -fno-builtin-fminl, because otherwise fminl
> is expanded inline and again there is no call left with the problem.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

No, I think the issue should be fixed in intrinsics.

Attached patch solves this problem for me, and also fixes a couple of
similar problems (one with -m3dnowa, that is nowadays a regular
compile option). The patched intrinsics were tested with combinations
of -m{,no-}sse{,2}, -m{,no-}mmx, -m{-no}3dnow{,a}, -m64, and there
were no problems with any combination.

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 2463 bytes --]

diff --git a/gcc/config/i386/mm3dnow.h b/gcc/config/i386/mm3dnow.h
index c8a91a1..2d5c538 100644
--- a/gcc/config/i386/mm3dnow.h
+++ b/gcc/config/i386/mm3dnow.h
@@ -30,9 +30,13 @@
 #include <mmintrin.h>
 #include <prfchwintrin.h>
 
-#ifndef __3dNOW__
+#if defined __x86_64__ && !defined __SSE__ || !defined __3dNOW__
 #pragma GCC push_options
+#ifdef __x86_64__
+#pragma GCC target("sse,3dnow")
+#else
 #pragma GCC target("3dnow")
+#endif
 #define __DISABLE_3dNOW__
 #endif /* __3dNOW__ */
 
@@ -176,7 +180,20 @@ _m_to_float (__m64 __A)
   return __tmp.a[0];
 }
 
-#ifdef __3dNOW_A__
+#ifdef __DISABLE_3dNOW__
+#undef __DISABLE_3dNOW__
+#pragma GCC pop_options
+#endif /* __DISABLE_3dNOW__ */
+
+#if defined __x86_64__ && !defined __SSE__ || !defined __3dNOW_A__
+#pragma GCC push_options
+#ifdef __x86_64__
+#pragma GCC target("sse,3dnowa")
+#else
+#pragma GCC target("3dnowa")
+#endif
+#define __DISABLE_3dNOW_A__
+#endif /* __3dNOW_A__ */
 
 extern __inline __m64 __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 _m_pf2iw (__m64 __A)
@@ -208,11 +225,9 @@ _m_pswapd (__m64 __A)
   return (__m64)__builtin_ia32_pswapdsf ((__v2sf)__A);
 }
 
-#endif /* __3dNOW_A__ */
-
-#ifdef __DISABLE_3dNOW__
-#undef __DISABLE_3dNOW__
+#ifdef __DISABLE_3dNOW_A__
+#undef __DISABLE_3dNOW_A__
 #pragma GCC pop_options
-#endif /* __DISABLE_3dNOW__ */
+#endif /* __DISABLE_3dNOW_A__ */
 
 #endif /* _MM3DNOW_H_INCLUDED */
diff --git a/gcc/config/i386/mmintrin.h b/gcc/config/i386/mmintrin.h
index 957d766..2cb73e3 100644
--- a/gcc/config/i386/mmintrin.h
+++ b/gcc/config/i386/mmintrin.h
@@ -27,9 +27,13 @@
 #ifndef _MMINTRIN_H_INCLUDED
 #define _MMINTRIN_H_INCLUDED
 
-#ifndef __MMX__
+#if defined __x86_64__ && !defined __SSE__ || !defined __MMX__
 #pragma GCC push_options
+#ifdef __x86_64__
+#pragma GCC target("sse,mmx")
+#else
 #pragma GCC target("mmx")
+#endif
 #define __DISABLE_MMX__
 #endif /* __MMX__ */
 
@@ -311,7 +315,7 @@ _m_paddd (__m64 __m1, __m64 __m2)
 /* Add the 64-bit values in M1 to the 64-bit values in M2.  */
 #ifndef __SSE2__
 #pragma GCC push_options
-#pragma GCC target("sse2")
+#pragma GCC target("sse2,mmx")
 #define __DISABLE_SSE2__
 #endif /* __SSE2__ */
 
@@ -423,7 +427,7 @@ _m_psubd (__m64 __m1, __m64 __m2)
 /* Add the 64-bit values in M1 to the 64-bit values in M2.  */
 #ifndef __SSE2__
 #pragma GCC push_options
-#pragma GCC target("sse2")
+#pragma GCC target("sse2,mmx")
 #define __DISABLE_SSE2__
 #endif /* __SSE2__ */
 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  7:42 ` Uros Bizjak
@ 2017-04-05  8:00   ` Jakub Jelinek
  2017-04-05  8:12     ` Uros Bizjak
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2017-04-05  8:00 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches

On Wed, Apr 05, 2017 at 09:42:31AM +0200, Uros Bizjak wrote:
> On Tue, Apr 4, 2017 at 9:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > Hi!
> >
> > aggregate_value_p is called often very early during compilation, e.g.
> > from allocate_function or during gimplification of a call with lhs.
> > The problem with that is e.g. that on x86_64 -m64 -mno-sse we can't
> > include <x86intrin.h>, because the always_inline inline functions
> > in mmx and 3dnow intrinsic headers return __m64 or take __m64 as arguments
> > and that in the 64-bit ABI is in SSE register.
> >
> > The following patch makes sure we diagnose this only later (e.g. when
> > expanding a function to RTL or when expanding calls to other functions),
> > which means we don't diagnose e.g. inline functions that got successfully
> > inlined (because then there is really no function return in SSE or x87
> > reg) or e.g. for builtin calls if they are emitted inline rather than
> > as a library call (again, I think that is desirable).
> > I had to tweak a few tests because the reported line changed slightly,
> > and in the last test add -fno-builtin-fminl, because otherwise fminl
> > is expanded inline and again there is no call left with the problem.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> No, I think the issue should be fixed in intrinsics.

But then you can't use the mmx intrinsics in pure mmx non-sse code, even
when my patch allows that.  Say:
#include <x86intrin.h>

__attribute__((target ("nosse,mmx"))) void
mmx_only (__m64 *a, __m64 *b, __m64 *c)
{
  *a = _mm_packs_pu16 (*b, *c);
}

or without the attribute, but in -mno-sse -mmmx compilation.
This compiles just fine for -m32 and there is no reason why it couldn't
work similarly for -m64, when the intrinsic will be inlined there is no
return nor argument needed in SSE registers.
So in effect it makes MMX unusable for 64-bit code without SSE.
Or do we just declare that unsupported?  Of course, people shouldn't be
using MMX, especially not in 64-bit code.

Note, my patch apparently doesn't handle the above, there is still the
aggregate_value_p call in NRV.
So maybe we should also not error if:
(cfun && current_ir_type () == IR_GIMPLE && !currently_expanding_to_rtl)
or so.

	Jakub

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  8:00   ` Jakub Jelinek
@ 2017-04-05  8:12     ` Uros Bizjak
  2017-04-05  8:20       ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Uros Bizjak @ 2017-04-05  8:12 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

On Wed, Apr 5, 2017 at 10:00 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Apr 05, 2017 at 09:42:31AM +0200, Uros Bizjak wrote:
>> On Tue, Apr 4, 2017 at 9:24 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > Hi!
>> >
>> > aggregate_value_p is called often very early during compilation, e.g.
>> > from allocate_function or during gimplification of a call with lhs.
>> > The problem with that is e.g. that on x86_64 -m64 -mno-sse we can't
>> > include <x86intrin.h>, because the always_inline inline functions
>> > in mmx and 3dnow intrinsic headers return __m64 or take __m64 as arguments
>> > and that in the 64-bit ABI is in SSE register.
>> >
>> > The following patch makes sure we diagnose this only later (e.g. when
>> > expanding a function to RTL or when expanding calls to other functions),
>> > which means we don't diagnose e.g. inline functions that got successfully
>> > inlined (because then there is really no function return in SSE or x87
>> > reg) or e.g. for builtin calls if they are emitted inline rather than
>> > as a library call (again, I think that is desirable).
>> > I had to tweak a few tests because the reported line changed slightly,
>> > and in the last test add -fno-builtin-fminl, because otherwise fminl
>> > is expanded inline and again there is no call left with the problem.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> No, I think the issue should be fixed in intrinsics.
>
> But then you can't use the mmx intrinsics in pure mmx non-sse code, even
> when my patch allows that.  Say:
> #include <x86intrin.h>
>
> __attribute__((target ("nosse,mmx"))) void
> mmx_only (__m64 *a, __m64 *b, __m64 *c)
> {
>   *a = _mm_packs_pu16 (*b, *c);
> }
>
> or without the attribute, but in -mno-sse -mmmx compilation.
> This compiles just fine for -m32 and there is no reason why it couldn't
> work similarly for -m64, when the intrinsic will be inlined there is no
> return nor argument needed in SSE registers.
> So in effect it makes MMX unusable for 64-bit code without SSE.
> Or do we just declare that unsupported?  Of course, people shouldn't be
> using MMX, especially not in 64-bit code.

Oh, I forgot to point out that on x86_64 ABI specifies MMX values in
SSE registers. So, on x86_64, users should not compile MMX code with
-mno-sse.

The problem, as reported in the PR was that <x86intrin.h> can't be
used with -mno-sse, even when source code that includes header does't
use SSE or MMX on x86_64. But, when these intrinsics are *used*, we
should still warn on x86_64. So the warning in your example is
correct.

Uros.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  8:12     ` Uros Bizjak
@ 2017-04-05  8:20       ` Jakub Jelinek
  2017-04-05  8:26         ` Uros Bizjak
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2017-04-05  8:20 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches

On Wed, Apr 05, 2017 at 10:12:02AM +0200, Uros Bizjak wrote:
> Oh, I forgot to point out that on x86_64 ABI specifies MMX values in
> SSE registers.

I know it does.  And if people have their own function that returns
__m64 or takes such arguments, they surely have to.
The question is only about the case when no function (in the assembly)
returns in SSE registers nor gets arguments in them, when all the
MMX code is inside of a function.
With your patch, it is - the MMX intrinsics are functions and we error on
them even when they are inlined.
With my patch we count only the non-inlined functions, something we emit
assembly for or call them from other TUs.

If you think requiring SSE for MMX always in 64-bit code is fine, even
when not strictly needed (as in, you really don't need SSE ISA to execute
such code, although there are no CPUs without that HW), so be it, then
let's go with your patch.

	Jakub

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  8:20       ` Jakub Jelinek
@ 2017-04-05  8:26         ` Uros Bizjak
  2017-04-05  8:29           ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Uros Bizjak @ 2017-04-05  8:26 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

On Wed, Apr 5, 2017 at 10:20 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Apr 05, 2017 at 10:12:02AM +0200, Uros Bizjak wrote:
>> Oh, I forgot to point out that on x86_64 ABI specifies MMX values in
>> SSE registers.
>
> I know it does.  And if people have their own function that returns
> __m64 or takes such arguments, they surely have to.
> The question is only about the case when no function (in the assembly)
> returns in SSE registers nor gets arguments in them, when all the
> MMX code is inside of a function.
> With your patch, it is - the MMX intrinsics are functions and we error on
> them even when they are inlined.
> With my patch we count only the non-inlined functions, something we emit
> assembly for or call them from other TUs.
>
> If you think requiring SSE for MMX always in 64-bit code is fine, even
> when not strictly needed (as in, you really don't need SSE ISA to execute
> such code, although there are no CPUs without that HW), so be it, then
> let's go with your patch.

Yes, I think that we should consistently warn even in case intrinsic
is inlined. I'm afraid that otherwise we will have endless stream of
bugreports that something warns with -O0, but not -O1+, when LTO is
used, etc, etc... The rule is: When user uses (or intends to use) MMX
value (that goes in SSE reg on x86_64), -msse is needed on x86_64,
otherwise warning is emitted.

Uros.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  8:26         ` Uros Bizjak
@ 2017-04-05  8:29           ` Jakub Jelinek
  2017-04-05 15:37             ` Uros Bizjak
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2017-04-05  8:29 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches

On Wed, Apr 05, 2017 at 10:26:47AM +0200, Uros Bizjak wrote:
> On Wed, Apr 5, 2017 at 10:20 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, Apr 05, 2017 at 10:12:02AM +0200, Uros Bizjak wrote:
> >> Oh, I forgot to point out that on x86_64 ABI specifies MMX values in
> >> SSE registers.
> >
> > I know it does.  And if people have their own function that returns
> > __m64 or takes such arguments, they surely have to.
> > The question is only about the case when no function (in the assembly)
> > returns in SSE registers nor gets arguments in them, when all the
> > MMX code is inside of a function.
> > With your patch, it is - the MMX intrinsics are functions and we error on
> > them even when they are inlined.
> > With my patch we count only the non-inlined functions, something we emit
> > assembly for or call them from other TUs.
> >
> > If you think requiring SSE for MMX always in 64-bit code is fine, even
> > when not strictly needed (as in, you really don't need SSE ISA to execute
> > such code, although there are no CPUs without that HW), so be it, then
> > let's go with your patch.
> 
> Yes, I think that we should consistently warn even in case intrinsic
> is inlined. I'm afraid that otherwise we will have endless stream of
> bugreports that something warns with -O0, but not -O1+, when LTO is
> used, etc, etc... The rule is: When user uses (or intends to use) MMX
> value (that goes in SSE reg on x86_64), -msse is needed on x86_64,
> otherwise warning is emitted.

Well, error.  Anyway, if so, then please commit your patch.

	Jakub

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05  8:29           ` Jakub Jelinek
@ 2017-04-05 15:37             ` Uros Bizjak
  2017-04-06 18:37               ` Uros Bizjak
  0 siblings, 1 reply; 9+ messages in thread
From: Uros Bizjak @ 2017-04-05 15:37 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

On Wed, Apr 5, 2017 at 10:29 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Apr 05, 2017 at 10:26:47AM +0200, Uros Bizjak wrote:
>> On Wed, Apr 5, 2017 at 10:20 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Apr 05, 2017 at 10:12:02AM +0200, Uros Bizjak wrote:
>> >> Oh, I forgot to point out that on x86_64 ABI specifies MMX values in
>> >> SSE registers.
>> >
>> > I know it does.  And if people have their own function that returns
>> > __m64 or takes such arguments, they surely have to.
>> > The question is only about the case when no function (in the assembly)
>> > returns in SSE registers nor gets arguments in them, when all the
>> > MMX code is inside of a function.
>> > With your patch, it is - the MMX intrinsics are functions and we error on
>> > them even when they are inlined.
>> > With my patch we count only the non-inlined functions, something we emit
>> > assembly for or call them from other TUs.
>> >
>> > If you think requiring SSE for MMX always in 64-bit code is fine, even
>> > when not strictly needed (as in, you really don't need SSE ISA to execute
>> > such code, although there are no CPUs without that HW), so be it, then
>> > let's go with your patch.
>>
>> Yes, I think that we should consistently warn even in case intrinsic
>> is inlined. I'm afraid that otherwise we will have endless stream of
>> bugreports that something warns with -O0, but not -O1+, when LTO is
>> used, etc, etc... The rule is: When user uses (or intends to use) MMX
>> value (that goes in SSE reg on x86_64), -msse is needed on x86_64,
>> otherwise warning is emitted.
>
> Well, error.  Anyway, if so, then please commit your patch.

Committed with the following ChangeLog:

2017-04-05  Uros Bizjak  <ubizjak@gmail.com>

    PR target/80298
    * config/i386/mmintrin.h: Add -msse target option when __SSE__ is
    not defined for x86_64 target.  Add -mmmx target option when __SSE2__
    is not defined.
    * config/i386/mm3dnow.h: Add -msse target when __SSE__ is not defined
    for x86_64 target.  Handle -m3dnowa option.

I choose not to include testcases, since mm_malloc includes stdlib.h,
which uses SSE register return with -O2, resulting in:

In file included from /usr/include/stdlib.h:921:0,
                 from /ssd/uros/gcc-build/gcc/include/mm_malloc.h:27,
                 from /ssd/uros/gcc-build/gcc/include/xmmintrin.h:34,
                 from /ssd/uros/gcc-build/gcc/include/x86intrin.h:33,
                 from
/home/uros/gcc-svn/trunk/gcc/testsuite/gcc.target/i386/pr80298-2.c:5:
/usr/include/bits/stdlib-float.h: In function 'atof':
/usr/include/bits/stdlib-float.h:27:1: error: SSE register return with
SSE disabled
compiler exited with status 1

This issue is out of scope of the compiler.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Committed to mainline SVN.

Uros.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298)
  2017-04-05 15:37             ` Uros Bizjak
@ 2017-04-06 18:37               ` Uros Bizjak
  0 siblings, 0 replies; 9+ messages in thread
From: Uros Bizjak @ 2017-04-06 18:37 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

On Wed, Apr 5, 2017 at 5:37 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

> 2017-04-05  Uros Bizjak  <ubizjak@gmail.com>
>
>     PR target/80298
>     * config/i386/mmintrin.h: Add -msse target option when __SSE__ is
>     not defined for x86_64 target.  Add -mmmx target option when __SSE2__
>     is not defined.
>     * config/i386/mm3dnow.h: Add -msse target when __SSE__ is not defined
>     for x86_64 target.  Handle -m3dnowa option.
>
> I choose not to include testcases, since mm_malloc includes stdlib.h,
> which uses SSE register return with -O2, resulting in:

Including only <mm3dnow.h> and its dependant mmintrin.h tests the
issue as well while avoiding external include files.

2017-04-06  Uros Bizjak  <ubizjak@gmail.com>

    PR target/80298
    * gcc.target/i386/pr80298-1.c: New test.
    * gcc.target/i386/pr80298-2.c: Ditto.

Tested on x86_64-linux-gnu {,-m32} and committed to mainline SVN.

Uros.

Index: gcc.target/i386/pr80298-1.c
===================================================================
--- gcc.target/i386/pr80298-1.c (nonexistent)
+++ gcc.target/i386/pr80298-1.c (working copy)
@@ -0,0 +1,7 @@
+/* PR target/80298 */
+/* { dg-do compile } */
+/* { dg-options "-mno-sse -mmmx" } */
+
+#include <mm3dnow.h>
+
+int i;
Index: gcc.target/i386/pr80298-2.c
===================================================================
--- gcc.target/i386/pr80298-2.c (nonexistent)
+++ gcc.target/i386/pr80298-2.c (working copy)
@@ -0,0 +1,7 @@
+/* PR target/80298 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mno-sse -mmmx" } */
+
+#include <mm3dnow.h>
+
+int i;

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-04-06 18:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-04 19:24 [PATCH] Don't error about x86 return value in SSE reg (or x86 reg) or argument in SSE reg too early (PR target/80298) Jakub Jelinek
2017-04-05  7:42 ` Uros Bizjak
2017-04-05  8:00   ` Jakub Jelinek
2017-04-05  8:12     ` Uros Bizjak
2017-04-05  8:20       ` Jakub Jelinek
2017-04-05  8:26         ` Uros Bizjak
2017-04-05  8:29           ` Jakub Jelinek
2017-04-05 15:37             ` Uros Bizjak
2017-04-06 18:37               ` Uros Bizjak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).