Bug 85667 - ms_abi rules aren't followed when returning and passing short structs with float and double values
Summary: ms_abi rules aren't followed when returning and passing short structs with fl...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 10.0
: P2 normal
Target Milestone: 10.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ABI
Depends on:
Blocks: 88521
  Show dependency treegraph
 
Reported: 2018-05-06 01:21 UTC by Anthony Green
Modified: 2020-02-07 12:02 UTC (History)
6 users (show)

See Also:
Host:
Target: i?86-*-* x86_64-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-03-23 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Anthony Green 2018-05-06 01:21:22 UTC
Structs 8 bytes or shorter with floating point values should return in eax.

GCC 8.0.1 (at least) doesn't follow those rules.  It is using the sysv ABI rules instead.

See this code, for example:

typedef struct
{
  float x;
} Float;

Float  __attribute__((ms_abi)) fn1()
{
  Float v;
  v.x = 3.145;
  return v;
}

Float fn2 ()
{
  Float v;
  v.x = 3.145;
  return v;
}


GCC's output for fn1 and fn2 are similar:

fn1:
	movss	.LC0(%rip), %xmm0
	ret

fn2:
	movss	.LC0(%rip), %xmm0
	ret


Clang does this correctly, however, and fn1 looks like this:

fn1:
	movl	$1078544302, %eax
	retq

fn2: 
	movss	.LCPI1_0(%rip), %xmm0 
	retq
Comment 1 Vinay Kumar 2018-10-17 09:18:13 UTC
Hi,

The current exists in latest trunk sources also. Please let me know is there any progress on this.

Regards,
Vinay
Comment 2 uros 2018-11-21 20:10:29 UTC
Author: uros
Date: Wed Nov 21 20:09:56 2018
New Revision: 266355

URL: https://gcc.gnu.org/viewcvs?rev=266355&root=gcc&view=rev
Log:
	PR target/85667
	* config/i386/i386.c (function_value_ms_64): Return AX_REG instead
	of FIRST_SSE_REG for 4 or 8 byte modes.

testsuite/ChangeLog:

	PR target/85667
	* gcc.target/pr85667-1.c: New testcase.
	* gcc.target/pr85667-2.c: New testcase.
	* gcc.target/pr85667-3.c: New testcase.
	* gcc.target/pr85667-4.c: New testcase.


Added:
    trunk/gcc/testsuite/gcc.target/i386/pr85667-1.c
    trunk/gcc/testsuite/gcc.target/i386/pr85667-2.c
    trunk/gcc/testsuite/gcc.target/i386/pr85667-3.c
    trunk/gcc/testsuite/gcc.target/i386/pr85667-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog
Comment 3 Jakub Jelinek 2018-12-04 14:41:09 UTC
So fixed?
Comment 4 mateuszb 2018-12-19 23:18:09 UTC
Now gcc 9 is heavily broken for mingw-w64 target -- so it is not fixed.
Functions that returns float or double should return in xmm0 (not eax).

In gcc-9 source code there are some hints:
gcc/config/i386/cygming.h from line #400:
/* MSVC returns aggregate types of up to 8 bytes via registers.
   See i386.c:ix86_return_in_memory.  */
#undef MS_AGGREGATE_RETURN
#define MS_AGGREGATE_RETURN 1

I think we should check if type is aggregate before we return in eax and leave xmm0 for float and double, for example:
Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	(revision 267268)
+++ gcc/config/i386/i386.c	(working copy)
@@ -9063,6 +9063,13 @@
 	      && !COMPLEX_MODE_P (mode))
 	    regno = FIRST_SSE_REG;
 	  break;
+	case 8:
+	case 4:
+	  if (valtype != NULL_TREE && AGGREGATE_TYPE_P (valtype))
+	    break;
+	  if (mode == SFmode || mode == DFmode)
+	    regno = FIRST_SSE_REG;
+	  break;
 	default:
 	  break;
         }
Comment 5 Lokesh Janghel 2018-12-20 07:31:55 UTC
>>I think we should check if type is aggregate before we return in eax and >>leave xmm0 for float and double.
>> 	  break;
>>+	case 8:
>>+	case 4:
>>+	  if (valtype != NULL_TREE && AGGREGATE_TYPE_P (valtype))
>>+	    break;
>>+	  if (mode == SFmode || mode == DFmode)
>>+	    regno = FIRST_SSE_REG;
>>+	  break;

It is working in both targets (x86_64 & mingw-w64). I tested in both targets.
Comment 6 mateuszb 2019-01-07 11:30:24 UTC
Now it is fixed for 64-bit GCC 9.
For 32-bit GCC 9 it is not fixed (return goes to st(0)).
Comment 7 Eric Botcazou 2019-03-23 12:27:18 UTC
The original issue exists in both 32-bit and 64-bit modes.  r266355 was broken but got fixed by r267622 (PR target/88521) so the issue is fixed in 64-bit mode.
Comment 8 uros 2019-03-27 21:18:19 UTC
Author: uros
Date: Wed Mar 27 21:17:48 2019
New Revision: 269979

URL: https://gcc.gnu.org/viewcvs?rev=269979&root=gcc&view=rev
Log:
	PR target/85667
	* config/i386/i386.c (ix86_function_value_1): Call the newly added
	function for 32-bit MS_ABI.
	(function_value_ms_32): New function.

testsuite/ChangeLog:

	PR target/85667
	* gcc.target/i386/pr85667-5.c: New testcase.
	* gcc.target/i386/pr85667-6.c: New testcase.


Added:
    trunk/gcc/testsuite/gcc.target/i386/pr85667-5.c
    trunk/gcc/testsuite/gcc.target/i386/pr85667-6.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog
Comment 9 Uroš Bizjak 2019-03-27 21:20:47 UTC
Fixed.
Comment 10 H.J. Lu 2020-02-05 16:26:52 UTC
The bug isn't fixed:

[hjl@gnu-cfl-1 tmp]$ cat x.c
typedef struct
{
  float x;
} Float;

Float  __attribute__((ms_abi)) fn1(Float x, Float y)
{
  Float v;
  v.x = x.x + y.x;
  return v;
}
[hjl@gnu-cfl-1 tmp]$ gcc -S -O2 x.c
[hjl@gnu-cfl-1 tmp]$ cat x.s
	.file	"x.c"
	.text
	.p2align 4
	.globl	fn1
	.type	fn1, @function
fn1:
.LFB0:
	.cfi_startproc
	addss	%xmm1, %xmm0   <<<<< Wrong registers are used.
	movd	%xmm0, %eax
	ret
	.cfi_endproc
.LFE0:
	.size	fn1, .-fn1
	.ident	"GCC: (GNU) 9.2.1 20200123 (Red Hat 9.2.1-3)"
	.section	.note.GNU-stack,"",@progbits
[hjl@gnu-cfl-1 tmp]$ clang -S -O2 x.c
[hjl@gnu-cfl-1 tmp]$ cat x.s
	.text
	.file	"x.c"
	.globl	fn1                     # -- Begin function fn1
	.p2align	4, 0x90
	.type	fn1,@function
fn1:                                    # @fn1
	.cfi_startproc
# %bb.0:
	movd	%ecx, %xmm0
	movd	%edx, %xmm1
	addss	%xmm0, %xmm1
	movd	%xmm1, %eax
	retq
.Lfunc_end0:
	.size	fn1, .Lfunc_end0-fn1
	.cfi_endproc
                                        # -- End function

	.ident	"clang version 9.0.0 (Fedora 9.0.0-1.fc31)"
	.section	".note.GNU-stack","",@progbits
	.addrsig
[hjl@gnu-cfl-1 tmp]$
Comment 11 H.J. Lu 2020-02-05 16:28:20 UTC
Confirmed with GCC 10.
Comment 12 H.J. Lu 2020-02-05 16:43:59 UTC
I am testing this

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index ffda3e8fd21..f769cb8f75e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -3153,7 +3153,7 @@ function_arg_64 (const CUMULATIVE_ARGS *cum, machine_mode mode,
 
 static rtx
 function_arg_ms_64 (const CUMULATIVE_ARGS *cum, machine_mode mode,
-         machine_mode orig_mode, bool named,
+         machine_mode orig_mode, bool named, const_tree type,
          HOST_WIDE_INT bytes)
 {
   unsigned int regno;
@@ -3173,7 +3173,10 @@ function_arg_ms_64 (const CUMULATIVE_ARGS *cum, machine_mode mode,
   if (TARGET_SSE && (mode == SFmode || mode == DFmode))
     {
       if (named)
-  regno = cum->regno + FIRST_SSE_REG;
+  {
+    if (type == NULL_TREE || !AGGREGATE_TYPE_P (type))
+      regno = cum->regno + FIRST_SSE_REG;
+  }
       else
   {
     rtx t1, t2;
@@ -3253,7 +3256,8 @@ ix86_function_arg (cumulative_args_t cum_v, const function_arg_info &arg)
       enum calling_abi call_abi = cum ? cum->call_abi : ix86_abi;
 
       if (call_abi == MS_ABI)
-  reg = function_arg_ms_64 (cum, mode, arg.mode, arg.named, bytes);
+  reg = function_arg_ms_64 (cum, mode, arg.mode, arg.named,
+             arg.type, bytes);
       else
   reg = function_arg_64 (cum, mode, arg.mode, arg.type, arg.named);
     }
Comment 13 CVS Commits 2020-02-07 11:38:46 UTC
The master branch has been updated by H.J. Lu <hjl@gcc.gnu.org>:

https://gcc.gnu.org/g:ea5ca698dca15dc86b823661ac357a30b49dd0f6

commit r10-6503-gea5ca698dca15dc86b823661ac357a30b49dd0f6
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Feb 7 03:37:46 2020 -0800

    x86-64: Pass aggregates with only float/double in GPRs for MS_ABI
    
    MS_ABI requires passing aggregates with only float/double in integer
    registers as shown in the output from MSVC v19.10 at:
    
    https://godbolt.org/z/2NPygd
    
    This patch fixed:
    
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=54 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=54 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=55 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=55 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=56 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=56 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    
    in libffi testsuite.
    
    gcc/
    
    	PR target/85667
    	* config/i386/i386.c (function_arg_ms_64): Add a type argument.
    	Don't return aggregates with only SFmode and DFmode in SSE
    	register.
    	(ix86_function_arg): Pass arg.type to function_arg_ms_64.
    
    gcc/testsuite/
    
    	PR target/85667
    	* gcc.target/i386/pr85667-10.c: New test.
    	* gcc.target/i386/pr85667-7.c: Likewise.
    	* gcc.target/i386/pr85667-8.c: Likewise.
    	* gcc.target/i386/pr85667-9.c: Likewise.
Comment 14 CVS Commits 2020-02-07 11:56:54 UTC
The releases/gcc-9 branch has been updated by H.J. Lu <hjl@gcc.gnu.org>:

https://gcc.gnu.org/g:850c38f5f4158a157fa792ca0b20a5a17a3ff642

commit r9-8204-g850c38f5f4158a157fa792ca0b20a5a17a3ff642
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Feb 7 03:50:40 2020 -0800

    x86-64: Pass aggregates with only float/double in GPRs for MS_ABI
    
    MS_ABI requires passing aggregates with only float/double in integer
    registers as shown in the output from MSVC v19.10 at:
    
    https://godbolt.org/z/2NPygd
    
    This patch fixed:
    
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=54 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=54 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=55 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=55 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=56 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O0 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    FAIL: libffi.bhaible/test-callback.c -W -Wall -Wno-psabi -DDGTEST=56 -Wno-unused-variable -Wno-unused-parameter -Wno-unused-but-set-variable -Wno-uninitialized -O2 -DABI_NUM=FFI_GNUW64 -DABI_ATTR=MSABI execution test
    
    in libffi testsuite.
    
    gcc/
    
    	Backport from mainline
    	PR target/85667
    	* config/i386/i386.c (function_arg_ms_64): Add a type argument.
    	Don't return aggregates with only SFmode and DFmode in SSE
    	register.
    	(ix86_function_arg): Pass type to function_arg_ms_64.
    
    gcc/testsuite/
    
    	Backport from mainline
    	PR target/85667
    	* gcc.target/i386/pr85667-10.c: New test.
    	* gcc.target/i386/pr85667-7.c: Likewise.
    	* gcc.target/i386/pr85667-8.c: Likewise.
    	* gcc.target/i386/pr85667-9.c: Likewise.
    
    (cherry picked from commit ea5ca698dca15dc86b823661ac357a30b49dd0f6)
Comment 15 H.J. Lu 2020-02-07 12:02:14 UTC
Fixed for GCC 10 and GCC 9.3.