[i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Kirill Yukhin]

Hello,
Here is a patch to fix pr58269.
Actually this is not a full fix, but an obvious part.

ChangeLog entry:
2013-09-06  Kirill Yukhin  <kirill.yukhin@intel.com>

	PR target/58269
	* gcc/config/i386/i386.c (ix86_conditional_register_usage):
	Proper initialize extended SSE registers.

Bootstrap pass.

Ok for trunk?

--
Thanks, K

---
 gcc/config/i386/i386.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index a8d70bc..d6a40a8 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -4218,7 +4218,7 @@ ix86_conditional_register_usage (void)
 
   /* If AVX512F is disabled, squash the registers.  */
   if (! TARGET_AVX512F)
-    for (i = FIRST_EXT_REX_SSE_REG; i < LAST_EXT_REX_SSE_REG; i++)
+    for (i = FIRST_EXT_REX_SSE_REG; i <= LAST_EXT_REX_SSE_REG; i++)
       fixed_regs[i] = call_used_regs[i] = 1, reg_names[i] = "";
 }
 
-- 
1.7.11.7

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Jakub Jelinek]

On Fri, Sep 06, 2013 at 11:28:53AM +0200, Uros Bizjak wrote:
> On Fri, Sep 6, 2013 at 10:34 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> > Hello,
> > Here is a patch to fix pr58269.
> > Actually this is not a full fix, but an obvious part.
> >
> > ChangeLog entry:
> > 2013-09-06  Kirill Yukhin  <kirill.yukhin@intel.com>
> >
> >         PR target/58269
> >         * gcc/config/i386/i386.c (ix86_conditional_register_usage):
> >         Proper initialize extended SSE registers.
> 
> This is OK.

But please leave out gcc/ prefix from the ChangeLog entry.

	Jakub

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Uros Bizjak]

On Fri, Sep 6, 2013 at 10:34 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> Hello,
> Here is a patch to fix pr58269.
> Actually this is not a full fix, but an obvious part.
>
> ChangeLog entry:
> 2013-09-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>         PR target/58269
>         * gcc/config/i386/i386.c (ix86_conditional_register_usage):
>         Proper initialize extended SSE registers.

This is OK.

Thanks,
Uros.

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Iain Sandoe]

Hi Kirill,

Thanks for Ilya's input on the PR thread.

We've done some testing/checking across the Darwin versions last night and I've bootstrapped all langs including Ada, and tested the patch below (together with the fragment you posted earlier) on Darwin12.

>> On Fri, Sep 6, 2013 at 10:34 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:

>>> Here is a patch to fix pr58269.
>>> Actually this is not a full fix, but an obvious part.

Here's what I propose for the remainder of the fix (FAOD, I cannot approve the Darwin changes).

====

Darwin is supposed to follow the System V psABI for x86_64 and, AFAICT for Darwin9, 10, 11 and 12 it is complying for function calls involving xmm0-7 (additional float args are, indeed, placed on the stack).  Ergo, saving xmm8-15 in __builtin_apply_args() only consumes time and stack space - the content is not part of the call.

As a fall-back; if I've missed a subtlety and, for some reason, we need to adjust the xmm set saved for compatibility with an older (gcc-4.2 based) system compiler, then we can override SSE_REGPARM_MAX in i386/darwin*.h for the relevant system version.

Losing TARGET_MACHO conditional code is always nice :)

Mike: Actually, since this seems to have uncovered a pre-existing wrong code bug, perhaps (this part of the) fix should also be applied to open branches?

gcc:

	PR target/58269
	config/i386/i386.c (ix86_function_arg_regno_p): Make Darwin use the
	xmm register set described in the psABI.

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index a8d70bc..e68b3f9 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -5706,17 +5706,9 @@ ix86_function_arg_regno_p (int regno)
 		    && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX)));
     }
 
-  if (TARGET_MACHO)
-    {
-      if (SSE_REGNO_P (regno) && TARGET_SSE)
-        return true;
-    }
-  else
-    {
-      if (TARGET_SSE && SSE_REGNO_P (regno)
-          && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX))
-        return true;
-    }
+  if (TARGET_SSE && SSE_REGNO_P (regno)
+      && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX))
+    return true;
 
   /* TODO: The function should depend on current function ABI but
      builtins.c would need updating then. Therefore we use the

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Kirill Yukhin]

Hello,
On 06 Sep 11:29, Jakub Jelinek wrote:
> On Fri, Sep 06, 2013 at 11:28:53AM +0200, Uros Bizjak wrote:
> > This is OK.
> 
> But please leave out gcc/ prefix from the ChangeLog entry.

Thanks, checked into main trunk: http://gcc.gnu.org/ml/gcc-cvs/2013-09/msg00181.html
with fixed ChangeLog.

--
Thanks, K

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Mike Stump]

On Sep 6, 2013, at 3:04 AM, Iain Sandoe <iain@codesourcery.com> wrote:
> Thanks for Ilya's input on the PR thread.

> Here's what I propose for the remainder of the fix (FAOD, I cannot approve the Darwin changes).

Ok.

> Mike: Actually, since this seems to have uncovered a pre-existing wrong code bug, perhaps (this part of the) fix should also be applied to open branches?

Bad wrong code gen I think is suitable for the release branches.  I double checked llvm:

  llvm/lib/Target/X86/X86CallingConv.td

and it seems to match.

Re: [i386, AVX-512F, pr58269] Partial fix for PR58269: properly initialize last EXT REX SSE register.

[Jack Howarth]

On Fri, Sep 06, 2013 at 11:04:45AM +0100, Iain Sandoe wrote:
> Hi Kirill,
> 
> Thanks for Ilya's input on the PR thread.
> 
> We've done some testing/checking across the Darwin versions last night and I've bootstrapped all langs including Ada, and tested the patch below (together with the fragment you posted earlier) on Darwin12.
> 
> >> On Fri, Sep 6, 2013 at 10:34 AM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> 
> >>> Here is a patch to fix pr58269.
> >>> Actually this is not a full fix, but an obvious part.
> 
> Here's what I propose for the remainder of the fix (FAOD, I cannot approve the Darwin changes).
> 
> ====
> 
> Darwin is supposed to follow the System V psABI for x86_64 and, AFAICT for Darwin9, 10, 11 and 12 it is complying for function calls involving xmm0-7 (additional float args are, indeed, placed on the stack).  Ergo, saving xmm8-15 in __builtin_apply_args() only consumes time and stack space - the content is not part of the call.
> 
> As a fall-back; if I've missed a subtlety and, for some reason, we need to adjust the xmm set saved for compatibility with an older (gcc-4.2 based) system compiler, then we can override SSE_REGPARM_MAX in i386/darwin*.h for the relevant system version.
> 
> Losing TARGET_MACHO conditional code is always nice :)
> 
> Mike: Actually, since this seems to have uncovered a pre-existing wrong code bug, perhaps (this part of the) fix should also be applied to open branches?
> 
> gcc:
> 
> 	PR target/58269
> 	config/i386/i386.c (ix86_function_arg_regno_p): Make Darwin use the
> 	xmm register set described in the psABI.
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index a8d70bc..e68b3f9 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -5706,17 +5706,9 @@ ix86_function_arg_regno_p (int regno)
>  		    && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX)));
>      }
>  
> -  if (TARGET_MACHO)
> -    {
> -      if (SSE_REGNO_P (regno) && TARGET_SSE)
> -        return true;
> -    }
> -  else
> -    {
> -      if (TARGET_SSE && SSE_REGNO_P (regno)
> -          && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX))
> -        return true;
> -    }
> +  if (TARGET_SSE && SSE_REGNO_P (regno)
> +      && (regno < FIRST_SSE_REG + SSE_REGPARM_MAX))
> +    return true;
>  
>    /* TODO: The function should depend on current function ABI but
>       builtins.c would need updating then. Therefore we use the

Iain,
   Do you plan to commit this to gcc trunk to unbreak the darwin bootstrap for libobjc?
The change seems to work fine here on x86_64-apple-darwin12...

http://gcc.gnu.org/ml/gcc-testresults/2013-09/msg00610.html

          Jack