Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Uros Bizjak]

Hello!

> "pushq $imm32S" only takes 32bit signed extended immediate. You can't push
> 0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
> patch makes it signed.  OK for trunk/4.5/4.4?

No, see the comment in real.h:

/* IN is a REAL_VALUE_TYPE.  OUT is a long.  */
#define REAL_VALUE_TO_TARGET_SINGLE(IN, OUT) \
  ((OUT) = real_to_target (NULL, &(IN), mode_for_size (32, MODE_FLOAT, 0)))

Uros.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Uros Bizjak]

On Sat, Aug 7, 2010 at 12:11 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

>> "pushq $imm32S" only takes 32bit signed extended immediate. You can't push
>> 0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
>> patch makes it signed.  OK for trunk/4.5/4.4?
>
> No, see the comment in real.h:
>
> /* IN is a REAL_VALUE_TYPE.  OUT is a long.  */
> #define REAL_VALUE_TO_TARGET_SINGLE(IN, OUT) \
>  ((OUT) = real_to_target (NULL, &(IN), mode_for_size (32, MODE_FLOAT, 0)))

IMO, this is correct patch, to also generate correct extension on ILP32 hosts.

Index: i386.c
===================================================================
--- i386.c	(revision 162975)
+++ i386.c	(working copy)
@@ -12921,7 +12921,7 @@

       if (ASSEMBLER_DIALECT == ASM_ATT)
 	putc ('$', file);
-      fprintf (file, "0x%08lx", (long unsigned int) l);
+      fprintf (file, "0x%08llx", (unsigned long long) (int) l);
     }

   /* These float cases don't actually occur as immediate operands.  */

Uros.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[H.J. Lu]

On Sat, Aug 7, 2010 at 3:45 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sat, Aug 7, 2010 at 12:11 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>
>>> "pushq $imm32S" only takes 32bit signed extended immediate. You can't push
>>> 0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
>>> patch makes it signed.  OK for trunk/4.5/4.4?
>>
>> No, see the comment in real.h:
>>
>> /* IN is a REAL_VALUE_TYPE.  OUT is a long.  */
>> #define REAL_VALUE_TO_TARGET_SINGLE(IN, OUT) \
>>  ((OUT) = real_to_target (NULL, &(IN), mode_for_size (32, MODE_FLOAT, 0)))
>
> IMO, this is correct patch, to also generate correct extension on ILP32 hosts.
>
> Index: i386.c
> ===================================================================
> --- i386.c      (revision 162975)
> +++ i386.c      (working copy)
> @@ -12921,7 +12921,7 @@
>
>       if (ASSEMBLER_DIALECT == ASM_ATT)
>        putc ('$', file);
> -      fprintf (file, "0x%08lx", (long unsigned int) l);
> +      fprintf (file, "0x%08llx", (unsigned long long) (int) l);
>     }
>
>   /* These float cases don't actually occur as immediate operands.  */
>

SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
For everything else, we use 32bit unsigned int.  We don't want

movl	$0xffffffffbf800000, (%rsp)

How does this patch look?

-- 
H.J.
---
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 204211a..4914299 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12921,7 +12921,12 @@ ix86_print_operand (FILE *file, rtx x, int code)

       if (ASSEMBLER_DIALECT == ASM_ATT)
   putc ('$', file);
-      fprintf (file, "0x%08lx", (long unsigned int) l);
+      /* 'q' is only used on pushq which takes 32bit signed extended
+   immediate.  */
+      if (code == 'q')
+  fprintf (file, "%d", (int) l);
+      else
+  fprintf (file, "0x%08x", (unsigned int) l);
     }

   /* These float cases don't actually occur as immediate operands.  */

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Uros Bizjak]

On Sat, 2010-08-07 at 08:52 -0700, H.J. Lu wrote:
> On Sat, Aug 7, 2010 at 3:45 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> > On Sat, Aug 7, 2010 at 12:11 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> >>> "pushq $imm32S" only takes 32bit signed extended immediate. You can't push
> >>> 0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
> >>> patch makes it signed.  OK for trunk/4.5/4.4?
> >>
> >> No, see the comment in real.h:
> >>
> >> /* IN is a REAL_VALUE_TYPE.  OUT is a long.  */
> >> #define REAL_VALUE_TO_TARGET_SINGLE(IN, OUT) \
> >>  ((OUT) = real_to_target (NULL, &(IN), mode_for_size (32, MODE_FLOAT, 0)))
> >
> > IMO, this is correct patch, to also generate correct extension on ILP32 hosts.
> >
> > Index: i386.c
> > ===================================================================
> > --- i386.c      (revision 162975)
> > +++ i386.c      (working copy)
> > @@ -12921,7 +12921,7 @@
> >
> >       if (ASSEMBLER_DIALECT == ASM_ATT)
> >        putc ('$', file);
> > -      fprintf (file, "0x%08lx", (long unsigned int) l);
> > +      fprintf (file, "0x%08llx", (unsigned long long) (int) l);
> >     }
> >
> >   /* These float cases don't actually occur as immediate operands.  */
> >
> 
> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
> For everything else, we use 32bit unsigned int.  We don't want
> 
> movl	$0xffffffffbf800000, (%rsp)
> 
> How does this patch look?

Based on your patch, I think this is what we want:

Index: i386.c
===================================================================
--- i386.c	(revision 162975)
+++ i386.c	(working copy)
@@ -12921,7 +12921,11 @@
 
       if (ASSEMBLER_DIALECT == ASM_ATT)
 	putc ('$', file);
-      fprintf (file, "0x%08lx", (long unsigned int) l);
+      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
+      if (code == 'q')
+	fprintf (file, "0x%08llx", (unsigned long long) (int) l);
+      else
+	fprintf (file, "0x%08x", (unsigned int) l);
     }
 
   /* These float cases don't actually occur as immediate operands.  */

Uros.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[H.J. Lu]

On Sat, Aug 7, 2010 at 10:54 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sat, 2010-08-07 at 08:52 -0700, H.J. Lu wrote:
>> On Sat, Aug 7, 2010 at 3:45 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> > On Sat, Aug 7, 2010 at 12:11 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> >
>> >>> "pushq $imm32S" only takes 32bit signed extended immediate. You can't push
>> >>> 0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
>> >>> patch makes it signed.  OK for trunk/4.5/4.4?
>> >>
>> >> No, see the comment in real.h:
>> >>
>> >> /* IN is a REAL_VALUE_TYPE.  OUT is a long.  */
>> >> #define REAL_VALUE_TO_TARGET_SINGLE(IN, OUT) \
>> >>  ((OUT) = real_to_target (NULL, &(IN), mode_for_size (32, MODE_FLOAT, 0)))
>> >
>> > IMO, this is correct patch, to also generate correct extension on ILP32 hosts.
>> >
>> > Index: i386.c
>> > ===================================================================
>> > --- i386.c      (revision 162975)
>> > +++ i386.c      (working copy)
>> > @@ -12921,7 +12921,7 @@
>> >
>> >       if (ASSEMBLER_DIALECT == ASM_ATT)
>> >        putc ('$', file);
>> > -      fprintf (file, "0x%08lx", (long unsigned int) l);
>> > +      fprintf (file, "0x%08llx", (unsigned long long) (int) l);
>> >     }
>> >
>> >   /* These float cases don't actually occur as immediate operands.  */
>> >
>>
>> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
>> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
>> For everything else, we use 32bit unsigned int.  We don't want
>>
>> movl  $0xffffffffbf800000, (%rsp)
>>
>> How does this patch look?
>
> Based on your patch, I think this is what we want:
>
> Index: i386.c
> ===================================================================
> --- i386.c      (revision 162975)
> +++ i386.c      (working copy)
> @@ -12921,7 +12921,11 @@
>
>       if (ASSEMBLER_DIALECT == ASM_ATT)
>        putc ('$', file);
> -      fprintf (file, "0x%08lx", (long unsigned int) l);
> +      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
> +      if (code == 'q')
> +       fprintf (file, "0x%08llx", (unsigned long long) (int) l);
> +      else
> +       fprintf (file, "0x%08x", (unsigned int) l);
>     }
>

That has nothing to do with 64bit ABI. It is due to how pushq works
in hardware. We do wants to generate a 4byte immediate, not 8byte.


-- 
H.J.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Uros Bizjak]

On Sat, Aug 7, 2010 at 8:44 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
>>> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
>>> For everything else, we use 32bit unsigned int.  We don't want
>>>
>>> movl  $0xffffffffbf800000, (%rsp)
>>>
>>> How does this patch look?
>>
>> Based on your patch, I think this is what we want:
>>
>> Index: i386.c
>> ===================================================================
>> --- i386.c      (revision 162975)
>> +++ i386.c      (working copy)
>> @@ -12921,7 +12921,11 @@
>>
>>       if (ASSEMBLER_DIALECT == ASM_ATT)
>>        putc ('$', file);
>> -      fprintf (file, "0x%08lx", (long unsigned int) l);
>> +      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
>> +      if (code == 'q')
>> +       fprintf (file, "0x%08llx", (unsigned long long) (int) l);
>> +      else
>> +       fprintf (file, "0x%08x", (unsigned int) l);
>>     }
>>
>
> That has nothing to do with 64bit ABI. It is due to how pushq works
> in hardware. We do wants to generate a 4byte immediate, not 8byte.

OK will say "Sign extend 32bit signed immediate to 8 bytes.

0xff.ff.ff.ff.80.00.00.00

So, 8 bytes.

Uros.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[H.J. Lu]

On Sat, Aug 7, 2010 at 12:04 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sat, Aug 7, 2010 at 8:44 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
>>>> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
>>>> For everything else, we use 32bit unsigned int.  We don't want
>>>>
>>>> movl  $0xffffffffbf800000, (%rsp)
>>>>
>>>> How does this patch look?
>>>
>>> Based on your patch, I think this is what we want:
>>>
>>> Index: i386.c
>>> ===================================================================
>>> --- i386.c      (revision 162975)
>>> +++ i386.c      (working copy)
>>> @@ -12921,7 +12921,11 @@
>>>
>>>       if (ASSEMBLER_DIALECT == ASM_ATT)
>>>        putc ('$', file);
>>> -      fprintf (file, "0x%08lx", (long unsigned int) l);
>>> +      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
>>> +      if (code == 'q')
>>> +       fprintf (file, "0x%08llx", (unsigned long long) (int) l);
>>> +      else
>>> +       fprintf (file, "0x%08x", (unsigned int) l);
>>>     }
>>>
>>
>> That has nothing to do with 64bit ABI. It is due to how pushq works
>> in hardware. We do wants to generate a 4byte immediate, not 8byte.
>
> OK will say "Sign extend 32bit signed immediate to 8 bytes.
>
> 0xff.ff.ff.ff.80.00.00.00
>
> So, 8 bytes.

Only pushq sign extends to 8 bytes and we don't care about
the upper 4 bytes here.  Everything else is 4 byte.



-- 
H.J.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Uros Bizjak]

On Sat, 2010-08-07 at 14:09 -0700, H.J. Lu wrote:

> >>>> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
> >>>> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
> >>>> For everything else, we use 32bit unsigned int.  We don't want
> >>>>
> >>>> movl  $0xffffffffbf800000, (%rsp)
> >>>>
> >>>> How does this patch look?
> >>>
> >>> Based on your patch, I think this is what we want:
> >>>
> >>> Index: i386.c
> >>> ===================================================================
> >>> --- i386.c      (revision 162975)
> >>> +++ i386.c      (working copy)
> >>> @@ -12921,7 +12921,11 @@
> >>>
> >>>       if (ASSEMBLER_DIALECT == ASM_ATT)
> >>>        putc ('$', file);
> >>> -      fprintf (file, "0x%08lx", (long unsigned int) l);
> >>> +      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
> >>> +      if (code == 'q')
> >>> +       fprintf (file, "0x%08llx", (unsigned long long) (int) l);
> >>> +      else
> >>> +       fprintf (file, "0x%08x", (unsigned int) l);
> >>>     }
> >>>
> >>
> >> That has nothing to do with 64bit ABI. It is due to how pushq works
> >> in hardware. We do wants to generate a 4byte immediate, not 8byte.
> >
> > OK will say "Sign extend 32bit signed immediate to 8 bytes.
> >
> > 0xff.ff.ff.ff.80.00.00.00
> >
> > So, 8 bytes.
> 
> Only pushq sign extends to 8 bytes and we don't care about
> the upper 4 bytes here.  Everything else is 4 byte.

You have to feed pushq with 8 bytes, and we do care for upper 4 bytes
since they should all be 0xff to correctly sign-extend the 32bit
immediate.

pushq mnemonic _REQUIRES_ 8 bytes, the top 4 must be explicitly set in
the asm source. Just try to assemble "pushq 0xfeffffff80000000".

Uros.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[H.J. Lu]

On Sat, Aug 7, 2010 at 2:23 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sat, 2010-08-07 at 14:09 -0700, H.J. Lu wrote:
>
>> >>>> SF is 4 bytes, the same as SI. pushq takes 32bit signed extended immediate.
>> >>>> It is safe for pushq since the upper 4 bytes, which are all 1s, are unused.
>> >>>> For everything else, we use 32bit unsigned int.  We don't want
>> >>>>
>> >>>> movl  $0xffffffffbf800000, (%rsp)
>> >>>>
>> >>>> How does this patch look?
>> >>>
>> >>> Based on your patch, I think this is what we want:
>> >>>
>> >>> Index: i386.c
>> >>> ===================================================================
>> >>> --- i386.c      (revision 162975)
>> >>> +++ i386.c      (working copy)
>> >>> @@ -12921,7 +12921,11 @@
>> >>>
>> >>>       if (ASSEMBLER_DIALECT == ASM_ATT)
>> >>>        putc ('$', file);
>> >>> -      fprintf (file, "0x%08lx", (long unsigned int) l);
>> >>> +      /* For 64bit ABI sign extend 32bit immediate to 8 bytes.  */
>> >>> +      if (code == 'q')
>> >>> +       fprintf (file, "0x%08llx", (unsigned long long) (int) l);
>> >>> +      else
>> >>> +       fprintf (file, "0x%08x", (unsigned int) l);
>> >>>     }
>> >>>
>> >>
>> >> That has nothing to do with 64bit ABI. It is due to how pushq works
>> >> in hardware. We do wants to generate a 4byte immediate, not 8byte.
>> >
>> > OK will say "Sign extend 32bit signed immediate to 8 bytes.
>> >
>> > 0xff.ff.ff.ff.80.00.00.00
>> >
>> > So, 8 bytes.
>>
>> Only pushq sign extends to 8 bytes and we don't care about
>> the upper 4 bytes here.  Everything else is 4 byte.
>
> You have to feed pushq with 8 bytes, and we do care for upper 4 bytes
> since they should all be 0xff to correctly sign-extend the 32bit
> immediate.
>
> pushq mnemonic _REQUIRES_ 8 bytes, the top 4 must be explicitly set in
> the asm source. Just try to assemble "pushq 0xfeffffff80000000".
>

This is probably too much details. pushq puts 8byte, which is sign extended
from 32bit immediate operand, on stack. pushq takes INT_MIN to INT_MAX.
You can express them in either 8byte hex or 4byte signed decimal. They
are equivalent as long as 8byte hex is within INT_MIN and INT_MAX.

Since SF is 4byte and the size of each argument gets rounded up to eightbytes,
it is OK to use pushq to put SF on stack when passing function parameter.
The upper 4bytes of "pushq" target are unused by callee.

-- 
H.J.

Re: PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[Richard Henderson]

On 08/06/2010 03:38 PM, H.J. Lu wrote:
> 	PR target/45213
> 	* config/i386/i386.c (ix86_print_operand): Use int instead of
> 	long with REAL_VALUE_TO_TARGET_SINGLE.

Surely we can also simplify the bits following to

  fprintf (file, "%d", l);


r~

PATCH: PR target/45213: "suffix or operands invalid for `push'" triggered by optimisations on x86_64

[H.J. Lu]

Hi,

"pushq $imm32S" only takes 32bit signed extended immediate. You can't push
0xbf800000. Instead, you push -1082130432 or 0xffffffffbf800000.  This
patch makes it signed.  OK for trunk/4.5/4.4?

Thanks.


H.J.
---
gcc/

2010-08-08  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/45213
	* config/i386/i386.c (ix86_print_operand): Use int instead of
	long with REAL_VALUE_TO_TARGET_SINGLE.

gcc/testsuite/

2010-08-08  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/45213
	* gcc.target/i386/pr45213.c.

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 204211a..f461481 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -12914,7 +12914,7 @@ ix86_print_operand (FILE *file, rtx x, int code)
   else if (GET_CODE (x) == CONST_DOUBLE && GET_MODE (x) == SFmode)
     {
       REAL_VALUE_TYPE r;
-      long l;
+      int l;
 
       REAL_VALUE_FROM_CONST_DOUBLE (r, x);
       REAL_VALUE_TO_TARGET_SINGLE (r, l);
--- /dev/null	2010-07-23 13:04:30.193381062 -0700
+++ gcc/gcc/testsuite/gcc.target/i386/pr45213.c	2010-08-06 15:15:03.680353825 -0700
@@ -0,0 +1,9 @@
+/* PR target/45213 */
+/* { dg-do assemble } */
+/* { dg-options "-Os -fno-omit-frame-pointer" } */
+
+void f(float, float, float, float, float, float, float, float, float, float);
+
+void g() {
+  f(0,0,0,0, 0,0,0,0, -1,1);
+}