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

login
register
mail settings
Submitter Richard Sandiford
Date Nov. 5, 2012, 8:30 a.m.
Message ID <87wqy0cuzt.fsf@talisman.home>
Download mbox | patch
Permalink /patch/197162/
State New
Headers show

Comments

Richard Sandiford - Nov. 5, 2012, 8:30 a.m.
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.
Uros Bizjak - Nov. 5, 2012, 9:01 a.m.
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.
H.J. Lu - Nov. 5, 2012, 10:21 a.m.
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.
H.J. Lu - Nov. 5, 2012, 12:54 p.m.
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.
Richard Sandiford - Nov. 5, 2012, 6:56 p.m.
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
H.J. Lu - Nov. 5, 2012, 11:34 p.m.
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.

Patch

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;