[x86] Fix gcc.c-torture/compile/20080806-1.c failures

[x86] Fix gcc.c-torture/compile/20080806-1.c failures

[Richard Sandiford]

This is a case where we had:

  (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X)))
  (clobber (reg:CC FLAGS_REG))

and the splitters decided to convert it to an LEA:

  (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X)))

But this fails to match, because ix86_address_subreg_operand
doesn't allow subregs of the stack pointer.

This shows up an inconsistency in the way the generic code handles
subregs of the stack pointer.  Normally we refuse to fold them, even
after reload, but the final_p case of alter_subreg folds them anyway.
That's how we ended up with the rather odd 16-bit sp.

However, even if the special alter_subreg case was removed
(so that we continued to use stack_pointer_rtx itself), we'd have:

  (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X)))
  (clobber (reg:CC FLAGS_REG))

which would get converted to:

  (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X)))

and we'd ICE in the same way.

The reason x86 rejects subregs of the stack pointer is this same refusal
to fold.  ix86_print_operand_address tries to simplify a SUBREG to a REG
and simplify_subreg wouldn't do anything for sp.

simplify_subreg isn't a lot of help at the output stage though.
If the insn stream contains a subreg that could be simplified but
hasn't been, then IMO that's a bug.  The cases we have to handle here
are those that can't be simplified (unless we decide at some point that
all registers must be simplifiable after reload, in which case we shouldn't
need to handle SUBREGs at all).

As things stand, I think we should be using true_regnum in this case instead.

Tested on x86_64-linux-gnu.  OK to install?

Richard


gcc/
	PR target/55204
	* config/i386/i386.c (ix86_address_subreg_operand): Remove stack
	pointer check.
	(print_reg): Use true_regnum rather than REGNO.
	(ix86_print_operand_address): Remove SUBREG handling.

Index: gcc/config/i386/i386.c
===================================================================
--- gcc/config/i386/i386.c	2012-11-04 21:53:46.000000000 +0000
+++ gcc/config/i386/i386.c	2012-11-04 21:53:46.821356709 +0000
@@ -11784,10 +11784,6 @@ ix86_address_subreg_operand (rtx op)
   if (GET_MODE_SIZE (mode) > UNITS_PER_WORD)
     return false;
 
-  /* simplify_subreg does not handle stack pointer.  */
-  if (REGNO (op) == STACK_POINTER_REGNUM)
-    return false;
-
   /* Allow only SUBREGs of non-eliminable hard registers.  */
   return register_no_elim_operand (op, mode);
 }
@@ -14084,13 +14080,6 @@ print_reg (rtx x, int code, FILE *file)
   const char *reg;
   bool duplicated = code == 'd' && TARGET_AVX;
 
-  gcc_assert (x == pc_rtx
-	      || (REGNO (x) != ARG_POINTER_REGNUM
-		  && REGNO (x) != FRAME_POINTER_REGNUM
-		  && REGNO (x) != FLAGS_REG
-		  && REGNO (x) != FPSR_REG
-		  && REGNO (x) != FPCR_REG));
-
   if (ASSEMBLER_DIALECT == ASM_ATT)
     putc ('%', file);
 
@@ -14101,6 +14090,13 @@ print_reg (rtx x, int code, FILE *file)
       return;
     }
 
+  unsigned int regno = true_regnum (x);
+  gcc_assert (regno != ARG_POINTER_REGNUM
+	      && regno != FRAME_POINTER_REGNUM
+	      && regno != FLAGS_REG
+	      && regno != FPSR_REG
+	      && regno != FPCR_REG);
+
   if (code == 'w' || MMX_REG_P (x))
     code = 2;
   else if (code == 'b')
@@ -14126,7 +14122,7 @@ print_reg (rtx x, int code, FILE *file)
     {
       gcc_assert (TARGET_64BIT);
       putc ('r', file);
-      fprint_ul (file, REGNO (x) - FIRST_REX_INT_REG + 8);
+      fprint_ul (file, regno - FIRST_REX_INT_REG + 8);
       switch (code)
 	{
 	  case 0:
@@ -14170,24 +14166,24 @@ print_reg (rtx x, int code, FILE *file)
     case 16:
     case 2:
     normal:
-      reg = hi_reg_name[REGNO (x)];
+      reg = hi_reg_name[regno];
       break;
     case 1:
-      if (REGNO (x) >= ARRAY_SIZE (qi_reg_name))
+      if (regno >= ARRAY_SIZE (qi_reg_name))
 	goto normal;
-      reg = qi_reg_name[REGNO (x)];
+      reg = qi_reg_name[regno];
       break;
     case 0:
-      if (REGNO (x) >= ARRAY_SIZE (qi_high_reg_name))
+      if (regno >= ARRAY_SIZE (qi_high_reg_name))
 	goto normal;
-      reg = qi_high_reg_name[REGNO (x)];
+      reg = qi_high_reg_name[regno];
       break;
     case 32:
       if (SSE_REG_P (x))
 	{
 	  gcc_assert (!duplicated);
 	  putc ('y', file);
-	  fputs (hi_reg_name[REGNO (x)] + 1, file);
+	  fputs (hi_reg_name[regno] + 1, file);
 	  return;
 	}
       break;
@@ -14943,22 +14939,6 @@ ix86_print_operand_address (FILE *file,
 
   gcc_assert (ok);
 
-  if (parts.base && GET_CODE (parts.base) == SUBREG)
-    {
-      rtx tmp = SUBREG_REG (parts.base);
-      parts.base = simplify_subreg (GET_MODE (parts.base),
-				    tmp, GET_MODE (tmp), 0);
-      gcc_assert (parts.base != NULL_RTX);
-    }
-
-  if (parts.index && GET_CODE (parts.index) == SUBREG)
-    {
-      rtx tmp = SUBREG_REG (parts.index);
-      parts.index = simplify_subreg (GET_MODE (parts.index),
-				     tmp, GET_MODE (tmp), 0);
-      gcc_assert (parts.index != NULL_RTX);
-    }
-
   base = parts.base;
   index = parts.index;
   disp = parts.disp;

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[Uros Bizjak]

On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> This is a case where we had:
>
>   (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X)))
>   (clobber (reg:CC FLAGS_REG))
>
> and the splitters decided to convert it to an LEA:
>
>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X)))
>
> But this fails to match, because ix86_address_subreg_operand
> doesn't allow subregs of the stack pointer.
>
> This shows up an inconsistency in the way the generic code handles
> subregs of the stack pointer.  Normally we refuse to fold them, even
> after reload, but the final_p case of alter_subreg folds them anyway.
> That's how we ended up with the rather odd 16-bit sp.
>
> However, even if the special alter_subreg case was removed
> (so that we continued to use stack_pointer_rtx itself), we'd have:
>
>   (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X)))
>   (clobber (reg:CC FLAGS_REG))
>
> which would get converted to:
>
>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X)))
>
> and we'd ICE in the same way.
>
> The reason x86 rejects subregs of the stack pointer is this same refusal
> to fold.  ix86_print_operand_address tries to simplify a SUBREG to a REG
> and simplify_subreg wouldn't do anything for sp.
>
> simplify_subreg isn't a lot of help at the output stage though.
> If the insn stream contains a subreg that could be simplified but
> hasn't been, then IMO that's a bug.  The cases we have to handle here
> are those that can't be simplified (unless we decide at some point that
> all registers must be simplifiable after reload, in which case we shouldn't
> need to handle SUBREGs at all).
>
> As things stand, I think we should be using true_regnum in this case instead.
>
> Tested on x86_64-linux-gnu.  OK to install?

Let's ask H.J. to test this change on x32.

> gcc/
>         PR target/55204
>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>         pointer check.
>         (print_reg): Use true_regnum rather than REGNO.
>         (ix86_print_operand_address): Remove SUBREG handling.

The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
-maddress-mode={short,long} on x32.

> +  unsigned int regno = true_regnum (x);

I'd rather see the declaration at the beginning of the function.

Thanks,
Uros.

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[H.J. Lu]

On Mon, Nov 5, 2012 at 1:01 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> This is a case where we had:
>>
>>   (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X)))
>>   (clobber (reg:CC FLAGS_REG))
>>
>> and the splitters decided to convert it to an LEA:
>>
>>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X)))
>>
>> But this fails to match, because ix86_address_subreg_operand
>> doesn't allow subregs of the stack pointer.
>>
>> This shows up an inconsistency in the way the generic code handles
>> subregs of the stack pointer.  Normally we refuse to fold them, even
>> after reload, but the final_p case of alter_subreg folds them anyway.
>> That's how we ended up with the rather odd 16-bit sp.
>>
>> However, even if the special alter_subreg case was removed
>> (so that we continued to use stack_pointer_rtx itself), we'd have:
>>
>>   (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X)))
>>   (clobber (reg:CC FLAGS_REG))
>>
>> which would get converted to:
>>
>>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X)))
>>
>> and we'd ICE in the same way.
>>
>> The reason x86 rejects subregs of the stack pointer is this same refusal
>> to fold.  ix86_print_operand_address tries to simplify a SUBREG to a REG
>> and simplify_subreg wouldn't do anything for sp.
>>
>> simplify_subreg isn't a lot of help at the output stage though.
>> If the insn stream contains a subreg that could be simplified but
>> hasn't been, then IMO that's a bug.  The cases we have to handle here
>> are those that can't be simplified (unless we decide at some point that
>> all registers must be simplifiable after reload, in which case we shouldn't
>> need to handle SUBREGs at all).
>>
>> As things stand, I think we should be using true_regnum in this case instead.
>>
>> Tested on x86_64-linux-gnu.  OK to install?
>
> Let's ask H.J. to test this change on x32.
>
>> gcc/
>>         PR target/55204
>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>         pointer check.
>>         (print_reg): Use true_regnum rather than REGNO.
>>         (ix86_print_operand_address): Remove SUBREG handling.
>
> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
> -maddress-mode={short,long} on x32.
>

I am on it.  It will take a while.

H.J.

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[H.J. Lu]

On Mon, Nov 5, 2012 at 2:21 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 5, 2012 at 1:01 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> This is a case where we had:
>>>
>>>   (set (reg:HI foo) (plus:HI (reg:HI sp) (const_int X)))
>>>   (clobber (reg:CC FLAGS_REG))
>>>
>>> and the splitters decided to convert it to an LEA:
>>>
>>>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:HI sp) 0) (const_int X)))
>>>
>>> But this fails to match, because ix86_address_subreg_operand
>>> doesn't allow subregs of the stack pointer.
>>>
>>> This shows up an inconsistency in the way the generic code handles
>>> subregs of the stack pointer.  Normally we refuse to fold them, even
>>> after reload, but the final_p case of alter_subreg folds them anyway.
>>> That's how we ended up with the rather odd 16-bit sp.
>>>
>>> However, even if the special alter_subreg case was removed
>>> (so that we continued to use stack_pointer_rtx itself), we'd have:
>>>
>>>   (set (reg:HI foo) (plus:HI (subreg:HI (reg:DI sp) 0) (const_int X)))
>>>   (clobber (reg:CC FLAGS_REG))
>>>
>>> which would get converted to:
>>>
>>>   (set (reg:SI foo) (plus:SI (subreg:SI (reg:DI sp) 0) (const_int X)))
>>>
>>> and we'd ICE in the same way.
>>>
>>> The reason x86 rejects subregs of the stack pointer is this same refusal
>>> to fold.  ix86_print_operand_address tries to simplify a SUBREG to a REG
>>> and simplify_subreg wouldn't do anything for sp.
>>>
>>> simplify_subreg isn't a lot of help at the output stage though.
>>> If the insn stream contains a subreg that could be simplified but
>>> hasn't been, then IMO that's a bug.  The cases we have to handle here
>>> are those that can't be simplified (unless we decide at some point that
>>> all registers must be simplifiable after reload, in which case we shouldn't
>>> need to handle SUBREGs at all).
>>>
>>> As things stand, I think we should be using true_regnum in this case instead.
>>>
>>> Tested on x86_64-linux-gnu.  OK to install?
>>
>> Let's ask H.J. to test this change on x32.
>>
>>> gcc/
>>>         PR target/55204
>>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>>         pointer check.
>>>         (print_reg): Use true_regnum rather than REGNO.
>>>         (ix86_print_operand_address): Remove SUBREG handling.
>>
>> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
>> -maddress-mode={short,long} on x32.
>>
>
> I am on it.  It will take a while.
>

I tested it with -maddress-mode={short,long} on x32.  There are
no regressions in GCC nor glibc testsuite.

Thanks.

-- 
H.J.

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[Richard Sandiford]

Uros Bizjak <ubizjak@gmail.com> writes:
> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> gcc/
>>         PR target/55204
>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>         pointer check.
>>         (print_reg): Use true_regnum rather than REGNO.
>>         (ix86_print_operand_address): Remove SUBREG handling.
>
> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
> -maddress-mode={short,long} on x32.
>
>> +  unsigned int regno = true_regnum (x);
>
> I'd rather see the declaration at the beginning of the function.

OK, thanks, applied to both with that change.

Richard

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[H.J. Lu]

On Mon, Nov 5, 2012 at 10:56 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Uros Bizjak <ubizjak@gmail.com> writes:
>> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> gcc/
>>>         PR target/55204
>>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>>         pointer check.
>>>         (print_reg): Use true_regnum rather than REGNO.
>>>         (ix86_print_operand_address): Remove SUBREG handling.
>>
>> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
>> -maddress-mode={short,long} on x32.
>>
>>> +  unsigned int regno = true_regnum (x);
>>
>> I'd rather see the declaration at the beginning of the function.
>
> OK, thanks, applied to both with that change.
>
> Richard


On 4.7 branch, this failed to build x32 run-time library:

/tmp/ccID3vr6.s: Assembler messages:
/tmp/ccID3vr6.s:2788: Error: bad register name `%rr15'

There is an extra 'r' in register name.

-- 
H.J.

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[H.J. Lu]

On Mon, Nov 5, 2012 at 3:34 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 5, 2012 at 10:56 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Uros Bizjak <ubizjak@gmail.com> writes:
>>> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>> gcc/
>>>>         PR target/55204
>>>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>>>         pointer check.
>>>>         (print_reg): Use true_regnum rather than REGNO.
>>>>         (ix86_print_operand_address): Remove SUBREG handling.
>>>
>>> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
>>> -maddress-mode={short,long} on x32.
>>>
>>>> +  unsigned int regno = true_regnum (x);
>>>
>>> I'd rather see the declaration at the beginning of the function.
>>
>> OK, thanks, applied to both with that change.
>>
>> Richard
>
>
> On 4.7 branch, this failed to build x32 run-time library:
>
> /tmp/ccID3vr6.s: Assembler messages:
> /tmp/ccID3vr6.s:2788: Error: bad register name `%rr15'
>
> There is an extra 'r' in register name.
>
> --
> H.J.

I think this is a real bug:

    case 8:
    case 4:
    case 12:
      if (! ANY_FP_REG_P (x))
        putc (code == 8 && TARGET_64BIT ? 'r' : 'e', file);
      /* FALLTHRU */
    case 16:
    case 2:
    normal:
      reg = hi_reg_name[regno];
      break;

hi_reg_name has

#define HI_REGISTER_NAMES                                               \
{"ax","dx","cx","bx","si","di","bp","sp",                               \
 "st","st(1)","st(2)","st(3)","st(4)","st(5)","st(6)","st(7)",          \
 "argp", "flags", "fpsr", "fpcr", "frame",                              \
 "xmm0","xmm1","xmm2","xmm3","xmm4","xmm5","xmm6","xmm7",               \
 "mm0", "mm1", "mm2", "mm3", "mm4", "mm5", "mm6", "mm7",                \
 "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",                  \
 "xmm8", "xmm9", "xmm10", "xmm11", "xmm12", "xmm13", "xmm14", "xmm15"}

If we ever use r8 to r15, we print wrong register name.  We should
catch it with

  /* Irritatingly, AMD extended registers use different naming convention
     from the normal registers: "r%d[bwd]"  */
  if (REX_INT_REG_P (x))

But it doesn't work with subreg change.   I am testing this

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 37498ef..b42870f 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -13729,7 +13729,7 @@ print_reg (rtx x, int code, FILE *file)

   /* Irritatingly, AMD extended registers use different naming convention
      from the normal registers: "r%d[bwd]"  */
-  if (REX_INT_REG_P (x))
+  if (REX_INT_REGNO_P (regno))
     {
       gcc_assert (TARGET_64BIT);
       putc ('r', file);

I will check it into trunk and 4.7 branch if test passes.

Thanks.


-- 
H.J.

Re: [x86] Fix gcc.c-torture/compile/20080806-1.c failures

[Richard Sandiford]

"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Mon, Nov 5, 2012 at 3:34 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Nov 5, 2012 at 10:56 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Uros Bizjak <ubizjak@gmail.com> writes:
>>>> On Mon, Nov 5, 2012 at 9:30 AM, Richard Sandiford
>>>> <rdsandiford@googlemail.com> wrote:
>>>>> gcc/
>>>>>         PR target/55204
>>>>>         * config/i386/i386.c (ix86_address_subreg_operand): Remove stack
>>>>>         pointer check.
>>>>>         (print_reg): Use true_regnum rather than REGNO.
>>>>>         (ix86_print_operand_address): Remove SUBREG handling.
>>>>
>>>> The patch is OK for mainline and 4.7, if it passes H.J.'s tests with
>>>> -maddress-mode={short,long} on x32.
>>>>
>>>>> +  unsigned int regno = true_regnum (x);
>>>>
>>>> I'd rather see the declaration at the beginning of the function.
>>>
>>> OK, thanks, applied to both with that change.
>>>
>>> Richard
>>
>>
>> On 4.7 branch, this failed to build x32 run-time library:
>>
>> /tmp/ccID3vr6.s: Assembler messages:
>> /tmp/ccID3vr6.s:2788: Error: bad register name `%rr15'
>>
>> There is an extra 'r' in register name.

Argh!  Interesting that you only hit this on 4.7.  Perhaps that's
an indication that 4.8 is better at not leaving unsimplified subregs
in the insn stream.

> If we ever use r8 to r15, we print wrong register name.  We should
> catch it with
>
>   /* Irritatingly, AMD extended registers use different naming convention
>      from the normal registers: "r%d[bwd]"  */
>   if (REX_INT_REG_P (x))
>
> But it doesn't work with subreg change.   I am testing this
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 37498ef..b42870f 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -13729,7 +13729,7 @@ print_reg (rtx x, int code, FILE *file)
>
>    /* Irritatingly, AMD extended registers use different naming convention
>       from the normal registers: "r%d[bwd]"  */
> -  if (REX_INT_REG_P (x))
> +  if (REX_INT_REGNO_P (regno))
>      {
>        gcc_assert (TARGET_64BIT);
>        putc ('r', file);
>
> I will check it into trunk and 4.7 branch if test passes.

Thanks HJ.

Richard