https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

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

Hi All,

The attached patch (pr85667.patch) fixes the subjected issue .
we tested on x86_64(linux and windows both) and no regress found .

ok to commit ?

Thank you
~Umesh

[-- Attachment #2: pr85667.patch --]
[-- Type: application/octet-stream, Size: 1725 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 1a70b8f..f385e24 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>
+
+	PR  target/85667
+	* i386.c (function_value_ms_64): ms_abi insist don't use the sse
+	(xmm0) register when returning structs whose size is less than 
+	or equal to the 8 byte.
+
 2018-11-14  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/87985
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 76a92b1..70dfc98 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
 	case 8:
 	case 4:
 	  if (mode == SFmode || mode == DFmode)
-	    regno = FIRST_SSE_REG;
+	    regno = AX_REG;
 	  break;
 	default:
 	  break;
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 50e53f0..5b76261 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>
+
+	PR target/85667
+	* gcc.target/pr85667.c: New testcase.
+ 
 2018-11-14  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/87985
diff --git a/gcc/testsuite/gcc.target/i386/pr85667.c b/gcc/testsuite/gcc.target/i386/pr85667.c
new file mode 100644
index 0000000..657a0f0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667.c
@@ -0,0 +1,20 @@
+/* { dg-options "-O2 " } */
+/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
+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;
+}

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85667

[Umesh Kalappa]

Edited the subjected for the proper PR no.
~Umesh

On Thu, Nov 15, 2018 at 2:32 PM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
>
> Hi All,
>
> The attached patch (pr85667.patch) fixes the subjected issue .
> we tested on x86_64(linux and windows both) and no regress found .
>
> ok to commit ?
>
> Thank you
> ~Umesh

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Richard Biener]

On Thu, Nov 15, 2018 at 10:02 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
>
> Hi All,
>
> The attached patch (pr85667.patch) fixes the subjected issue .
> we tested on x86_64(linux and windows both) and no regress found .
>
> ok to commit ?

I wonder if you can turn the testcase into a dg-run one, making the
functions noinline/noipa and check the correct values are returned.

Richard.

> Thank you
> ~Umesh

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

Thank you Richard,

Made the required changes ,ok to commit ?

Thank you
~Umesh
On Thu, Nov 15, 2018 at 4:02 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Nov 15, 2018 at 10:02 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
> >
> > Hi All,
> >
> > The attached patch (pr85667.patch) fixes the subjected issue .
> > we tested on x86_64(linux and windows both) and no regress found .
> >
> > ok to commit ?
>
> I wonder if you can turn the testcase into a dg-run one, making the
> functions noinline/noipa and check the correct values are returned.
>
> Richard.
>
> > Thank you
> > ~Umesh

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Richard Biener]

On Fri, Nov 16, 2018 at 9:07 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
>
> Thank you Richard,
>
> Made the required changes ,ok to commit ?

Can you attach the adjusted patch?

Thanks,
Richard.

> Thank you
> ~Umesh
> On Thu, Nov 15, 2018 at 4:02 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Thu, Nov 15, 2018 at 10:02 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
> > >
> > > Hi All,
> > >
> > > The attached patch (pr85667.patch) fixes the subjected issue .
> > > we tested on x86_64(linux and windows both) and no regress found .
> > >
> > > ok to commit ?
> >
> > I wonder if you can turn the testcase into a dg-run one, making the
> > functions noinline/noipa and check the correct values are returned.
> >
> > Richard.
> >
> > > Thank you
> > > ~Umesh

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

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

My bad ,
attached the same now .

~Umesh
On Fri, Nov 16, 2018 at 2:38 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Fri, Nov 16, 2018 at 9:07 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
> >
> > Thank you Richard,
> >
> > Made the required changes ,ok to commit ?
>
> Can you attach the adjusted patch?
>
> Thanks,
> Richard.
>
> > Thank you
> > ~Umesh
> > On Thu, Nov 15, 2018 at 4:02 PM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Thu, Nov 15, 2018 at 10:02 AM Umesh Kalappa <umesh.kalappa0@gmail.com> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > The attached patch (pr85667.patch) fixes the subjected issue .
> > > > we tested on x86_64(linux and windows both) and no regress found .
> > > >
> > > > ok to commit ?
> > >
> > > I wonder if you can turn the testcase into a dg-run one, making the
> > > functions noinline/noipa and check the correct values are returned.
> > >
> > > Richard.
> > >
> > > > Thank you
> > > > ~Umesh

[-- Attachment #2: pr85667.patch --]
[-- Type: application/octet-stream, Size: 1835 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 1a70b8f..3bba89c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,10 @@
+2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>
+
+	PR  target/85667
+	* i386.c (function_value_ms_64): ms_abi insist to use the eax
+	register, when function returning struct with size less than 
+	or equal to the 8 byte.
+
 2018-11-14  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/87985
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 76a92b1..70dfc98 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
 	case 8:
 	case 4:
 	  if (mode == SFmode || mode == DFmode)
-	    regno = FIRST_SSE_REG;
+	    regno = AX_REG;
 	  break;
 	default:
 	  break;
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 50e53f0..ec54330 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>
+
+	PR target/85667
+	* gcc.dg/pr85667.c: New testcase.
+ 
 2018-11-14  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/87985
diff --git a/gcc/testsuite/gcc.dg/pr85667.c b/gcc/testsuite/gcc.dg/pr85667.c
new file mode 100644
index 0000000..cc99602
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr85667.c
@@ -0,0 +1,29 @@
+/* { dg-options "-O2 " } */
+/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
+void abort ();
+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;
+}
+int main ()
+{
+  Float a, b;
+  a = fn1 ();
+  b = fn2 ();
+  if (a.x == 3.145 && b.x == 3.145)
+    return 0;
+  abort ();   
+}

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Jakub Jelinek]

On Fri, Nov 16, 2018 at 04:21:25PM +0530, Umesh Kalappa wrote:
> My bad ,
> attached the same now .

+2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>

Two spaces before < instead of just one.
+
+	PR  target/85667

Only a single space between PR and target.

+	* i386.c (function_value_ms_64): ms_abi insist to use the eax

The filename is relative to the directory with ChangeLog file, so
	* config/i386/i386.c
in this case.  The description should say what you've changed, so
something like:
	* config/i386/i386.c (function_value_ms_64): Return AX_REG instead
	of FIRST_SSE_REG for 4 or 8 byte modes.

--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
 	case 8:
 	case 4:
 	  if (mode == SFmode || mode == DFmode)
-	    regno = FIRST_SSE_REG;
+	    regno = AX_REG;
 	  break;

Is there something to back that up, say godbolt.org link with some testcases
showing how does MSVC, clang etc. handle those?
And, because the function starts with:
  unsigned int regno = AX_REG;
the change isn't right, you should remove all of:
        case 8:
        case 4:
          if (mode == SFmode || mode == DFmode)
            regno = FIRST_SSE_REG;
          break;
because the default will do what you want.

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 50e53f0..ec54330 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>

Two spaces between date and name.

--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr85667.c
@@ -0,0 +1,29 @@
+/* { dg-options "-O2 " } */
+/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */

First of all, the test is misplaced, it is clearly x86_64 specific
and probably lp64 only, so it shouldn't be in gcc.dg/ where it will
be run on all targets, but in gcc.target/i386/ and be guarded with
{ target lp64 }.

Second, seems like you'd like to run the testcase, so you'd better have it
/* { dg-do run { target lp64 } } */

The assembler scanning will work only with -masm=att, not -masm=intel
and seems to be very fragile, so I'd suggest have one runtime test and one
compile time test in which you put just the fn1 function.  Why two arbitrary
letters after dot?  That makes on sense.  Either you are looking for .LC\[0-9]*
specifically, or for arbitrary symbol, then use something like
"movl\[^\n\r]*, %eax"
or so (and make sure to use -masm=intel).

More interesting would be
make check ALT_CC_UNDER_TEST=msvc ALT_CXX_UNDER_TEST=msvc++ RUNTESTFLAGS='compat.exp struct-layout-1.exp'
(or whatever MSVC driver names are), i.e. try to run the compat testsuites
between MSVC and newly built gcc.

	Jakub

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Lokesh Janghel]

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

Thank you Jakub, I update the patch with your comments and tested it.
Please let me know your thoughts/suggestions.

On Mon, Nov 19, 2018 at 4:00 PM Lokesh Janghel
<lokeshjanghel91@gmail.com> wrote:
>
> Thank you Jakub, I update the patch with your comments and tested it.
> Please let me know your thoughts/suggestions.
>
>
> Thanks
> Lokesh
>
> On Fri, Nov 16, 2018 at 4:57 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Fri, Nov 16, 2018 at 04:21:25PM +0530, Umesh Kalappa wrote:
>> > My bad ,
>> > attached the same now .
>>
>> +2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>
>>
>> Two spaces before < instead of just one.
>> +
>> +       PR  target/85667
>>
>> Only a single space between PR and target.
>>
>> +       * i386.c (function_value_ms_64): ms_abi insist to use the eax
>>
>> The filename is relative to the directory with ChangeLog file, so
>>         * config/i386/i386.c
>> in this case.  The description should say what you've changed, so
>> something like:
>>         * config/i386/i386.c (function_value_ms_64): Return AX_REG instead
>>         of FIRST_SSE_REG for 4 or 8 byte modes.
>>
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
>>         case 8:
>>         case 4:
>>           if (mode == SFmode || mode == DFmode)
>> -           regno = FIRST_SSE_REG;
>> +           regno = AX_REG;
>>           break;
>>
>> Is there something to back that up, say godbolt.org link with some testcases
>> showing how does MSVC, clang etc. handle those?
>> And, because the function starts with:
>>   unsigned int regno = AX_REG;
>> the change isn't right, you should remove all of:
>>         case 8:
>>         case 4:
>>           if (mode == SFmode || mode == DFmode)
>>             regno = FIRST_SSE_REG;
>>           break;
>> because the default will do what you want.
>>
>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>> index 50e53f0..ec54330 100644
>> --- a/gcc/testsuite/ChangeLog
>> +++ b/gcc/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>
>>
>> Two spaces between date and name.
>>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr85667.c
>> @@ -0,0 +1,29 @@
>> +/* { dg-options "-O2 " } */
>> +/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
>>
>> First of all, the test is misplaced, it is clearly x86_64 specific
>> and probably lp64 only, so it shouldn't be in gcc.dg/ where it will
>> be run on all targets, but in gcc.target/i386/ and be guarded with
>> { target lp64 }.
>>
>> Second, seems like you'd like to run the testcase, so you'd better have it
>> /* { dg-do run { target lp64 } } */
>>
>> The assembler scanning will work only with -masm=att, not -masm=intel
>> and seems to be very fragile, so I'd suggest have one runtime test and one
>> compile time test in which you put just the fn1 function.  Why two arbitrary
>> letters after dot?  That makes on sense.  Either you are looking for .LC\[0-9]*
>> specifically, or for arbitrary symbol, then use something like
>> "movl\[^\n\r]*, %eax"
>> or so (and make sure to use -masm=intel).
>>
>> More interesting would be
>> make check ALT_CC_UNDER_TEST=msvc ALT_CXX_UNDER_TEST=msvc++ RUNTESTFLAGS='compat.exp struct-layout-1.exp'
>> (or whatever MSVC driver names are), i.e. try to run the compat testsuites
>> between MSVC and newly built gcc.
>>
>>         Jakub
>
>
>
> --
> Thanks & Regards
> Lokesh Janghel
> +91-9752984749



-- 
Thanks & Regards
Lokesh Janghel
+91-9752984749

[-- Attachment #2: 85667.patch --]
[-- Type: application/octet-stream, Size: 2448 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 8ca2e73..b55dfa9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-19 Lokesh Janghel <lokeshjanghel91@gmail.com>
+
+	* config/i386/i386.c(function_value_ms_64): Return AX_REG instead
+	of FIRST_SSE_REG for 4 or 8 byte modes.
+
 2018-11-16  Ilya Leoshkevich  <iii@linux.ibm.com>
 
 	* config/s390/s390.md
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c18c60a..41def54 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9005,11 +9005,6 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
 	      && !COMPLEX_MODE_P (mode))
 	    regno = FIRST_SSE_REG;
 	  break;
-	case 8:
-	case 4:
-	  if (mode == SFmode || mode == DFmode)
-	    regno = FIRST_SSE_REG;
-	  break;
 	default:
 	  break;
         }
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 647ea5d..40ec605 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2018-11-19 Lokesh Janghel <lokeshjanghel91@gmail.com>
+	
+	PR target/85667
+	* gcc.target/pr85667-1.c: New testcase.
+	* gcc.target/pr85667-2.c: New testcase.
+
 2018-11-16  Ilya Leoshkevich  <iii@linux.ibm.com>
 
 	* gcc.target/s390/md/rXsbg_mode_sXl.c: Do not use arithmetic in
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-1.c b/gcc/testsuite/gcc.target/i386/pr85667-1.c
new file mode 100644
index 0000000..cb3acbb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-1.c
@@ -0,0 +1,28 @@
+/* { dg-do run { target lp64 } } */
+void abort ();
+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;
+}
+int main ()
+{
+  Float a, b;
+  a = fn1 ();
+  b = fn2 ();
+  if (a.x == 3.145f && b.x == 3.145f)
+    return 0; 
+  abort ();   
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-2.c b/gcc/testsuite/gcc.target/i386/pr85667-2.c
new file mode 100644
index 0000000..b782c7d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-2.c
@@ -0,0 +1,15 @@
+/* { dg-do assemble } */
+/* { dg-options "-O2 -masm=intel" } */
+/* { dg-require-effective-target lp64 } */
+/* { dg-require-effective-target masm_intel } */ 
+/* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax" 1} } */
+typedef struct
+{
+  float x;
+} Float;
+Float __attribute__((ms_abi)) fn1 ()
+{
+  Float v;
+  v.x = 3.145;
+  return v;
+}

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Jakub Jelinek]

On Mon, Nov 19, 2018 at 04:08:29PM +0530, Lokesh Janghel wrote:
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 8ca2e73..b55dfa9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-19 Lokesh Janghel <lokeshjanghel91@gmail.com>

Two spaces between date and name and name and <, i.e.
2018-11-20  Lokesh Janghel  <lokeshjanghel91@gmail.com>
in both ChangeLog files.

--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-2.c
@@ -0,0 +1,15 @@
+/* { dg-do assemble } */
+/* { dg-options "-O2 -masm=intel" } */
+/* { dg-require-effective-target lp64 } */
+/* { dg-require-effective-target masm_intel } */ 
+/* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax" 1} } */
+typedef struct
+{
+  float x;
+} Float;
+Float __attribute__((ms_abi)) fn1 ()
+{
+  Float v;
+  v.x = 3.145;
+  return v;
+}

This test wasn't properly tested:

/usr/src/gcc/obj/gcc/xgcc -B/usr/src/gcc/obj/gcc/ -m64 -fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never -O2 -masm=intel -ffat-lto-objects -fno-ident -c -o pr85667-2.o /usr/src/gcc/gcc/testsuite/gcc.target/i386/pr85667-2.c
PASS: gcc.target/i386/pr85667-2.c (test for excess errors)
gcc.target/i386/pr85667-2.c: output file does not exist
UNRESOLVED: gcc.target/i386/pr85667-2.c scan-assembler-times movl[^\n\r]*, %eax 1
testcase /usr/src/gcc/gcc/testsuite/gcc.target/i386/i386.exp completed in 1 seconds

1) you do not want to use dg-do assemble, but dg-do compile, because only
   in that case (or when using -save-temps) assembly is produced
2) you do not want to use -masm=intel and then expect AT&T syntax in the
   regexp

Thus, I'd replace all the dg- directive lines with:
/* { dg-do compile { target lp64 } } */
/* { dg-options "-O2" } */
/* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax|mov\[ \t]*eax," 1 } } */

That way, it will work both with -masm=att (explicit or implicit) or
-masm=intel.

One can use

make check-gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64,-m64/-masm=intel\} i386.exp=pr85667*'

to verify and then look at the log file.

Furthermore, I'd copy pr85667-1.c test to pr85667-3.c and the modified
pr85667-2.c to pr85667-4.c, change Float to Double, float to double, remove
f suffixes and adjust all the eax in the regexp to rax, so that you also
test the struct with DFmode case.

	Jakub

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

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

Thank you for the inputs and please find the attachment for the update patch.

Do please let us know your comments on the same

~Umesh
On Tue, Nov 20, 2018 at 3:03 PM Jakub Jelinek <span> wrote:
&gt;
&gt; On Mon, Nov 19, 2018 at 04:08:29PM +0530, Lokesh Janghel wrote:
&gt; diff --git a/gcc/ChangeLog b/gcc/ChangeLog
&gt; index 8ca2e73..b55dfa9 100644
&gt; --- a/gcc/ChangeLog
&gt; +++ b/gcc/ChangeLog
&gt; @@ -1,3 +1,8 @@
&gt; +2018-11-19 Lokesh Janghel <span>
&gt;
&gt; Two spaces between date and name and name and &lt;, i.e.
&gt; 2018-11-20  Lokesh Janghel  <span>
&gt; in both ChangeLog files.
&gt;
&gt; --- /dev/null
&gt; +++ b/gcc/testsuite/gcc.target/i386/pr85667-2.c
&gt; @@ -0,0 +1,15 @@
&gt; +/* { dg-do assemble } */
&gt; +/* { dg-options "-O2 -masm=intel" } */
&gt; +/* { dg-require-effective-target lp64 } */
&gt; +/* { dg-require-effective-target masm_intel } */
&gt; +/* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax" 1} } */
&gt; +typedef struct
&gt; +{
&gt; +  float x;
&gt; +} Float;
&gt; +Float __attribute__((ms_abi)) fn1 ()
&gt; +{
&gt; +  Float v;
&gt; +  v.x = 3.145;
&gt; +  return v;
&gt; +}
&gt;
&gt; This test wasn't properly tested:
&gt;
&gt; /usr/src/gcc/obj/gcc/xgcc -B/usr/src/gcc/obj/gcc/ -m64
-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
-fdiagnostics-color=never -O2 -masm=intel -ffat-lto-objects -fno-ident
-c -o pr85667-2.o
/usr/src/gcc/gcc/testsuite/gcc.target/i386/pr85667-2.c
&gt; PASS: gcc.target/i386/pr85667-2.c (test for excess errors)
&gt; gcc.target/i386/pr85667-2.c: output file does not exist
&gt; UNRESOLVED: gcc.target/i386/pr85667-2.c scan-assembler-times
movl[^\n\r]*, %eax 1
&gt; testcase /usr/src/gcc/gcc/testsuite/gcc.target/i386/i386.exp
completed in 1 seconds
&gt;
&gt; 1) you do not want to use dg-do assemble, but dg-do compile, because only
&gt;    in that case (or when using -save-temps) assembly is produced
&gt; 2) you do not want to use -masm=intel and then expect AT&amp;T
syntax in the
&gt;    regexp
&gt;
&gt; Thus, I'd replace all the dg- directive lines with:
&gt; /* { dg-do compile { target lp64 } } */
&gt; /* { dg-options "-O2" } */
&gt; /* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax|mov\[
\t]*eax," 1 } } */
&gt;
&gt; That way, it will work both with -masm=att (explicit or implicit) or
&gt; -masm=intel.
&gt;
&gt; One can use
&gt;
&gt; make check-gcc
RUNTESTFLAGS='--target_board=unix\{-m32,-m64,-m64/-masm=intel\}
i386.exp=pr85667*'
&gt;
&gt; to verify and then look at the log file.
&gt;
&gt; Furthermore, I'd copy pr85667-1.c test to pr85667-3.c and the modified
&gt; pr85667-2.c to pr85667-4.c, change Float to Double, float to double, remove
&gt; f suffixes and adjust all the eax in the regexp to rax, so that you also
&gt; test the struct with DFmode case.
&gt;
&gt;         Jakub</span></span></span>

[-- Attachment #2: 85667.patch --]
[-- Type: application/octet-stream, Size: 3571 bytes --]

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 8ca2e73..f331185 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2018-11-19  Lokesh Janghel  <lokeshjanghel91@gmail.com>
+
+	* config/i386/i386.c(function_value_ms_64): Return AX_REG instead
+	of FIRST_SSE_REG for 4 or 8 byte modes.
+
 2018-11-16  Ilya Leoshkevich  <iii@linux.ibm.com>
 
 	* config/s390/s390.md
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c18c60a..41def54 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9005,11 +9005,6 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
 	      && !COMPLEX_MODE_P (mode))
 	    regno = FIRST_SSE_REG;
 	  break;
-	case 8:
-	case 4:
-	  if (mode == SFmode || mode == DFmode)
-	    regno = FIRST_SSE_REG;
-	  break;
 	default:
 	  break;
         }
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 647ea5d..76f2982 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,11 @@
+2018-11-19  Lokesh Janghel  <lokeshjanghel91@gmail.com>
+	
+	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.
+
 2018-11-16  Ilya Leoshkevich  <iii@linux.ibm.com>
 
 	* gcc.target/s390/md/rXsbg_mode_sXl.c: Do not use arithmetic in
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-1.c b/gcc/testsuite/gcc.target/i386/pr85667-1.c
new file mode 100644
index 0000000..cb3acbb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-1.c
@@ -0,0 +1,28 @@
+/* { dg-do run { target lp64 } } */
+void abort ();
+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;
+}
+int main ()
+{
+  Float a, b;
+  a = fn1 ();
+  b = fn2 ();
+  if (a.x == 3.145f && b.x == 3.145f)
+    return 0; 
+  abort ();   
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-2.c b/gcc/testsuite/gcc.target/i386/pr85667-2.c
new file mode 100644
index 0000000..a080d64
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler-times "movl\[^\n\r]*, %eax|mov\[ \t]*eax," 1 } } */
+typedef struct
+{
+  float x;
+} Float;
+Float __attribute__((ms_abi)) fn1 ()
+{
+  Float v;
+  v.x = 3.145;
+  return v;
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-3.c b/gcc/testsuite/gcc.target/i386/pr85667-3.c
new file mode 100644
index 0000000..6636e0e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-3.c
@@ -0,0 +1,28 @@
+/* { dg-do run { target lp64 } } */
+void abort ();
+typedef struct
+{
+  double x;
+} Double;
+
+Double __attribute__((ms_abi)) fn1 ()
+{
+  Double v;
+  v.x = 3.145;
+  return v;
+}
+Double fn2 ()
+{
+  Double v;
+  v.x = 3.145;
+  return v;
+}
+int main ()
+{
+  Double a, b;
+  a = fn1 ();
+  b = fn2 ();
+  if (a.x == 3.145 && b.x == 3.145)
+    return 0; 
+  abort ();   
+}
diff --git a/gcc/testsuite/gcc.target/i386/pr85667-4.c b/gcc/testsuite/gcc.target/i386/pr85667-4.c
new file mode 100644
index 0000000..7a98acb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr85667-4.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler-times "movq\[^\n\r]*, %rax|mov\[ \t]*rax," 1 } } */
+typedef struct
+{
+ double x;
+}Double;
+
+Double  __attribute__((ms_abi)) fn1 ()
+{
+  Double v;
+  v.x = 3.145;
+  return v;
+}

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Jakub Jelinek]

On Wed, Nov 21, 2018 at 06:06:41PM +0530, Umesh Kalappa wrote:
> Thank you for the inputs and please find the attachment for the update patch.

LGTM.

	Jakub

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

Hi Jakub and All,

We don't have the commit access ,can  someone please commit for us ?

~Umesh

On Wed, Nov 21, 2018, 18:37 Jakub Jelinek <jakub@redhat.com wrote:

> On Wed, Nov 21, 2018 at 06:06:41PM +0530, Umesh Kalappa wrote:
> > Thank you for the inputs and please find the attachment for the update
> patch.
>
> LGTM.
>
>         Jakub
>

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Jeff Law]

On 11/21/18 6:24 AM, Umesh Kalappa wrote:
> Hi Jakub and All,
> 
> We don't have the commit access ,can  someone please commit for us ?
Looks like Uros took care of it a few days ago.

jeff

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Martin Liška]

On 11/19/18 11:38 AM, Lokesh Janghel wrote:
> Thank you Jakub, I update the patch with your comments and tested it.
> Please let me know your thoughts/suggestions.
> 
> On Mon, Nov 19, 2018 at 4:00 PM Lokesh Janghel
> <lokeshjanghel91@gmail.com> wrote:
>>
>> Thank you Jakub, I update the patch with your comments and tested it.
>> Please let me know your thoughts/suggestions.
>>
>>
>> Thanks
>> Lokesh
>>
>> On Fri, Nov 16, 2018 at 4:57 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>>
>>> On Fri, Nov 16, 2018 at 04:21:25PM +0530, Umesh Kalappa wrote:
>>>> My bad ,
>>>> attached the same now .
>>>
>>> +2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>
>>>
>>> Two spaces before < instead of just one.
>>> +
>>> +       PR  target/85667
>>>
>>> Only a single space between PR and target.
>>>
>>> +       * i386.c (function_value_ms_64): ms_abi insist to use the eax
>>>
>>> The filename is relative to the directory with ChangeLog file, so
>>>          * config/i386/i386.c
>>> in this case.  The description should say what you've changed, so
>>> something like:
>>>          * config/i386/i386.c (function_value_ms_64): Return AX_REG instead
>>>          of FIRST_SSE_REG for 4 or 8 byte modes.
>>>
>>> --- a/gcc/config/i386/i386.c
>>> +++ b/gcc/config/i386/i386.c
>>> @@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
>>>          case 8:
>>>          case 4:
>>>            if (mode == SFmode || mode == DFmode)
>>> -           regno = FIRST_SSE_REG;
>>> +           regno = AX_REG;
>>>            break;
>>>
>>> Is there something to back that up, say godbolt.org link with some testcases
>>> showing how does MSVC, clang etc. handle those?
>>> And, because the function starts with:
>>>    unsigned int regno = AX_REG;
>>> the change isn't right, you should remove all of:
>>>          case 8:
>>>          case 4:
>>>            if (mode == SFmode || mode == DFmode)
>>>              regno = FIRST_SSE_REG;
>>>            break;
>>> because the default will do what you want.
>>>
>>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>>> index 50e53f0..ec54330 100644
>>> --- a/gcc/testsuite/ChangeLog
>>> +++ b/gcc/testsuite/ChangeLog
>>> @@ -1,3 +1,8 @@
>>> +2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>
>>>
>>> Two spaces between date and name.
>>>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.dg/pr85667.c
>>> @@ -0,0 +1,29 @@
>>> +/* { dg-options "-O2 " } */
>>> +/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
>>>
>>> First of all, the test is misplaced, it is clearly x86_64 specific
>>> and probably lp64 only, so it shouldn't be in gcc.dg/ where it will
>>> be run on all targets, but in gcc.target/i386/ and be guarded with
>>> { target lp64 }.
>>>
>>> Second, seems like you'd like to run the testcase, so you'd better have it
>>> /* { dg-do run { target lp64 } } */
>>>
>>> The assembler scanning will work only with -masm=att, not -masm=intel
>>> and seems to be very fragile, so I'd suggest have one runtime test and one
>>> compile time test in which you put just the fn1 function.  Why two arbitrary
>>> letters after dot?  That makes on sense.  Either you are looking for .LC\[0-9]*
>>> specifically, or for arbitrary symbol, then use something like
>>> "movl\[^\n\r]*, %eax"
>>> or so (and make sure to use -masm=intel).
>>>
>>> More interesting would be
>>> make check ALT_CC_UNDER_TEST=msvc ALT_CXX_UNDER_TEST=msvc++ RUNTESTFLAGS='compat.exp struct-layout-1.exp'
>>> (or whatever MSVC driver names are), i.e. try to run the compat testsuites
>>> between MSVC and newly built gcc.
>>>
>>>          Jakub
>>
>>
>>
>> --
>> Thanks & Regards
>> Lokesh Janghel
>> +91-9752984749
> 
> 
> 

Hi.

The patch causes failures in libffi:
https://github.com/libffi/libffi/issues/463

Can you please take a look?
Martin

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Jakub Jelinek]

On Sun, Jan 06, 2019 at 06:25:35PM +0100, Martin Liška wrote:
> The patch causes failures in libffi:
> https://github.com/libffi/libffi/issues/463
> 
> Can you please take a look?

Perhaps PR88521?  A patch has been posted for that and even approved:
https://gcc.gnu.org/ml/gcc-patches/2018-12/msg01682.html
but not actually committed yet.

	Jakub

Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87626

[Umesh Kalappa]

Hi Jakub ,

the attached patch is good to commit ?

Thank you
~Umesh
On Mon, Nov 19, 2018 at 4:00 PM Lokesh Janghel
<lokeshjanghel91@gmail.com> wrote:
>
> Thank you Jakub, I update the patch with your comments and tested it.
> Please let me know your thoughts/suggestions.
>
>
> Thanks
> Lokesh
>
> On Fri, Nov 16, 2018 at 4:57 PM Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> On Fri, Nov 16, 2018 at 04:21:25PM +0530, Umesh Kalappa wrote:
>> > My bad ,
>> > attached the same now .
>>
>> +2018-11-15  Lokesh Janghel <lokeshjanghel91@gmail.com>
>>
>> Two spaces before < instead of just one.
>> +
>> +       PR  target/85667
>>
>> Only a single space between PR and target.
>>
>> +       * i386.c (function_value_ms_64): ms_abi insist to use the eax
>>
>> The filename is relative to the directory with ChangeLog file, so
>>         * config/i386/i386.c
>> in this case.  The description should say what you've changed, so
>> something like:
>>         * config/i386/i386.c (function_value_ms_64): Return AX_REG instead
>>         of FIRST_SSE_REG for 4 or 8 byte modes.
>>
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -9008,7 +9008,7 @@ function_value_ms_64 (machine_mode orig_mode, machine_mode mode,
>>         case 8:
>>         case 4:
>>           if (mode == SFmode || mode == DFmode)
>> -           regno = FIRST_SSE_REG;
>> +           regno = AX_REG;
>>           break;
>>
>> Is there something to back that up, say godbolt.org link with some testcases
>> showing how does MSVC, clang etc. handle those?
>> And, because the function starts with:
>>   unsigned int regno = AX_REG;
>> the change isn't right, you should remove all of:
>>         case 8:
>>         case 4:
>>           if (mode == SFmode || mode == DFmode)
>>             regno = FIRST_SSE_REG;
>>           break;
>> because the default will do what you want.
>>
>> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
>> index 50e53f0..ec54330 100644
>> --- a/gcc/testsuite/ChangeLog
>> +++ b/gcc/testsuite/ChangeLog
>> @@ -1,3 +1,8 @@
>> +2018-11-15 Lokesh Janghel  <lokeshjanghel91@gmail.com>
>>
>> Two spaces between date and name.
>>
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pr85667.c
>> @@ -0,0 +1,29 @@
>> +/* { dg-options "-O2 " } */
>> +/* { dg-final { scan-assembler-times "movl\[\t \]\.\[a-zA-Z\]\[a-zA-Z\]\[0-9\]\\(%rip\\), %eax" 1} } */
>>
>> First of all, the test is misplaced, it is clearly x86_64 specific
>> and probably lp64 only, so it shouldn't be in gcc.dg/ where it will
>> be run on all targets, but in gcc.target/i386/ and be guarded with
>> { target lp64 }.
>>
>> Second, seems like you'd like to run the testcase, so you'd better have it
>> /* { dg-do run { target lp64 } } */
>>
>> The assembler scanning will work only with -masm=att, not -masm=intel
>> and seems to be very fragile, so I'd suggest have one runtime test and one
>> compile time test in which you put just the fn1 function.  Why two arbitrary
>> letters after dot?  That makes on sense.  Either you are looking for .LC\[0-9]*
>> specifically, or for arbitrary symbol, then use something like
>> "movl\[^\n\r]*, %eax"
>> or so (and make sure to use -masm=intel).
>>
>> More interesting would be
>> make check ALT_CC_UNDER_TEST=msvc ALT_CXX_UNDER_TEST=msvc++ RUNTESTFLAGS='compat.exp struct-layout-1.exp'
>> (or whatever MSVC driver names are), i.e. try to run the compat testsuites
>> between MSVC and newly built gcc.
>>
>>         Jakub
>
>
>
> --
> Thanks & Regards
> Lokesh Janghel
> +91-9752984749