RE: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

RE: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[David Sherwood]

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

Hi Vladimir,

Sorry this took so long. I have tidied up the patch as you suggested and fixed some
style issues. Hope this looks better now.

Thanks!
David.

2014-09-26  David Sherwood  <david.sherwood@arm.com>

        * ira-int.h (ira_allocno): Add "wmode" field.
        * ira-build.c (create_insn_allocnos): Add new "parent" function
        parameter.
        * ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
        that cannot be accessed in wmode.


-----Original Message-----
From: David Sherwood [mailto:david.sherwood@arm.com] 
Sent: 08 September 2014 12:48
To: 'gcc-patches@gcc.gnu.org'
Cc: 'vmakarov@redhat.com'
Subject: RE: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
compiler error)"

Hi Vladimir,

Sorry, I forgot to CC you on this as it's your code. It's my first attempt at
submitting patches to gcc so I'm still learning as I go!

Kind Regards,
David Sherwood.

-----Original Message-----
From: David Sherwood [mailto:david.sherwood@arm.com] 
Sent: 05 September 2014 15:52
To: 'gcc-patches@gcc.gnu.org'
Subject: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
compiler error)"

Hi,

I have a potential fix for a gcc testsuite failure for aarch64 in big endian, i.e.

FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)
FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_y_tst.o compile, (internal compiler error)

It is caused by the inappropriate choice of hard registers for paradoxical sub registers in
big endian mode, for example if register 0 is chosen for a paradoxical TI subreg on big
endian then we may end up attempting to reference register -1. Similarly, on little endian
we could end up going beyond the upper bounds of the register file too.

My fix involves adding particular constraints in IRA on the choice of register once paradoxical 
sub registers are encountered. However, Richard Sandiford also proposed an alternative
solution that involves not constraining registers in IRA, but rather making use of cost analysis
instead and letting LRA do the work. Not sure what your preference is ....

Fix was tested on aarch64 on little and big endian with no regressions.

Regards,
David Sherwood.

2014-08-26  David Sherwood  <david.sherwood@arm.com>

        * ira-int.h (ira_allocno): Add "wmode" field.
        * ira-build.c (create_insn_allocnos): Add new "parent" function
        parameter.
        * ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
        that cannot be accessed in wmode.

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

diff --git a/gcc/ira-build.c b/gcc/ira-build.c
index 3f2ab17..ddea1f6 100644
--- a/gcc/ira-build.c
+++ b/gcc/ira-build.c
@@ -524,6 +524,7 @@ ira_create_allocno (int regno, bool cap_p,
   ALLOCNO_BAD_SPILL_P (a) = false;
   ALLOCNO_ASSIGNED_P (a) = false;
   ALLOCNO_MODE (a) = (regno < 0 ? VOIDmode : PSEUDO_REGNO_MODE (regno));
+  ALLOCNO_WMODE (a) = ALLOCNO_MODE (a);
   ALLOCNO_PREFS (a) = NULL;
   ALLOCNO_COPIES (a) = NULL;
   ALLOCNO_HARD_REG_COSTS (a) = NULL;
@@ -893,6 +894,7 @@ create_cap_allocno (ira_allocno_t a)
   parent = ALLOCNO_LOOP_TREE_NODE (a)->parent;
   cap = ira_create_allocno (ALLOCNO_REGNO (a), true, parent);
   ALLOCNO_MODE (cap) = ALLOCNO_MODE (a);
+  ALLOCNO_WMODE (cap) = ALLOCNO_WMODE (a);
   aclass = ALLOCNO_CLASS (a);
   ira_set_allocno_class (cap, aclass);
   ira_create_allocno_objects (cap);
@@ -1859,9 +1861,11 @@ static basic_block curr_bb;
 
 /* This recursive function creates allocnos corresponding to
    pseudo-registers containing in X.  True OUTPUT_P means that X is
-   a lvalue.  */
+   a lvalue.  The 'parent' parameter corresponds to the parent expression
+   of 'rtx'.
+ */
 static void
-create_insn_allocnos (rtx x, bool output_p)
+create_insn_allocnos (rtx x, rtx outer, bool output_p)
 {
   int i, j;
   const char *fmt;
@@ -1876,7 +1880,15 @@ create_insn_allocnos (rtx x, bool output_p)
 	  ira_allocno_t a;
 
 	  if ((a = ira_curr_regno_allocno_map[regno]) == NULL)
-	    a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+            {
+              a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+              if (outer != NULL && GET_CODE (outer) == SUBREG)
+                {
+                  enum machine_mode wmode = GET_MODE (outer);
+                  if (GET_MODE_SIZE (wmode) > GET_MODE_SIZE (ALLOCNO_WMODE (a)))
+                    ALLOCNO_WMODE (a) = wmode;
+                }
+            }
 
 	  ALLOCNO_NREFS (a)++;
 	  ALLOCNO_FREQ (a) += REG_FREQ_FROM_BB (curr_bb);
@@ -1887,25 +1899,25 @@ create_insn_allocnos (rtx x, bool output_p)
     }
   else if (code == SET)
     {
-      create_insn_allocnos (SET_DEST (x), true);
-      create_insn_allocnos (SET_SRC (x), false);
+      create_insn_allocnos (SET_DEST (x), NULL, true);
+      create_insn_allocnos (SET_SRC (x), NULL, false);
       return;
     }
   else if (code == CLOBBER)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
       return;
     }
   else if (code == MEM)
     {
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
   else if (code == PRE_DEC || code == POST_DEC || code == PRE_INC ||
 	   code == POST_INC || code == POST_MODIFY || code == PRE_MODIFY)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
 
@@ -1913,10 +1925,10 @@ create_insn_allocnos (rtx x, bool output_p)
   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
     {
       if (fmt[i] == 'e')
-	create_insn_allocnos (XEXP (x, i), output_p);
+	create_insn_allocnos (XEXP (x, i), x, output_p);
       else if (fmt[i] == 'E')
 	for (j = 0; j < XVECLEN (x, i); j++)
-	  create_insn_allocnos (XVECEXP (x, i, j), output_p);
+	  create_insn_allocnos (XVECEXP (x, i, j), x, output_p);
     }
 }
 
@@ -1935,7 +1947,7 @@ create_bb_allocnos (ira_loop_tree_node_t bb_node)
   ira_assert (bb != NULL);
   FOR_BB_INSNS_REVERSE (bb, insn)
     if (NONDEBUG_INSN_P (insn))
-      create_insn_allocnos (PATTERN (insn), false);
+      create_insn_allocnos (PATTERN (insn), NULL, false);
   /* It might be a allocno living through from one subloop to
      another.  */
   EXECUTE_IF_SET_IN_REG_SET (df_get_live_in (bb), FIRST_PSEUDO_REGISTER, i, bi)
diff --git a/gcc/ira-conflicts.c b/gcc/ira-conflicts.c
index 011a865..ca71be2 100644
--- a/gcc/ira-conflicts.c
+++ b/gcc/ira-conflicts.c
@@ -774,6 +774,27 @@ ira_build_conflicts (void)
 				temp_hard_reg_set);
 	    }
 
+          /* Now we deal with paradoxical subreg cases where certain registers
+             cannot be accessed in the widest mode.  */
+          enum machine_mode outer_mode = ALLOCNO_WMODE (a);
+          enum machine_mode inner_mode = ALLOCNO_MODE (a);
+          if (GET_MODE_SIZE (outer_mode) > GET_MODE_SIZE (inner_mode))
+            {
+              enum reg_class aclass = ALLOCNO_CLASS (a);
+              for (int j = ira_class_hard_regs_num[aclass] - 1; j >= 0; --j)
+                {
+                   int inner_regno = ira_class_hard_regs[aclass][j];
+                   int outer_regno = simplify_subreg_regno (inner_regno,
+                                                            inner_mode, 0,
+                                                            outer_mode);
+                   if (outer_regno < 0 ||
+                       !in_hard_reg_set_p (reg_class_contents[aclass],
+                                           outer_mode, outer_regno))
+                     SET_HARD_REG_BIT (OBJECT_CONFLICT_HARD_REGS (obj),
+                                       inner_regno);
+                }
+            }
+
 	  if (ALLOCNO_CALLS_CROSSED_NUM (a) != 0)
 	    {
 	      int regno;
diff --git a/gcc/ira-int.h b/gcc/ira-int.h
index 1db0641..4035c6a 100644
--- a/gcc/ira-int.h
+++ b/gcc/ira-int.h
@@ -280,6 +280,9 @@ struct ira_allocno
   /* Mode of the allocno which is the mode of the corresponding
      pseudo-register.  */
   ENUM_BITFIELD (machine_mode) mode : 8;
+  /* Widest mode of the allocno which in at least one case could be
+     for paradoxical subregs where wmode > mode.  */
+  ENUM_BITFIELD (machine_mode) wmode : 8;
   /* Register class which should be used for allocation for given
      allocno.  NO_REGS means that we should use memory.  */
   ENUM_BITFIELD (reg_class) aclass : 16;
@@ -312,7 +315,7 @@ struct ira_allocno
      number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
      reload (at this point pseudo-register has only one allocno) which
      did not get stack slot yet.  */
-  short int hard_regno;
+  int hard_regno : 16;
   /* Allocnos with the same regno are linked by the following member.
      Allocnos corresponding to inner loops are first in the list (it
      corresponds to depth-first traverse of the loops).  */
@@ -433,6 +436,7 @@ struct ira_allocno
 #define ALLOCNO_BAD_SPILL_P(A) ((A)->bad_spill_p)
 #define ALLOCNO_ASSIGNED_P(A) ((A)->assigned_p)
 #define ALLOCNO_MODE(A) ((A)->mode)
+#define ALLOCNO_WMODE(A) ((A)->wmode)
 #define ALLOCNO_PREFS(A) ((A)->allocno_prefs)
 #define ALLOCNO_COPIES(A) ((A)->allocno_copies)
 #define ALLOCNO_HARD_REG_COSTS(A) ((A)->hard_reg_costs)

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Richard Sandiford]

"David Sherwood" <david.sherwood@arm.com> writes:
> @@ -1859,9 +1861,11 @@ static basic_block curr_bb;
>  
>  /* This recursive function creates allocnos corresponding to
>     pseudo-registers containing in X.  True OUTPUT_P means that X is
> -   a lvalue.  */
> +   a lvalue.  The 'parent' parameter corresponds to the parent expression
> +   of 'rtx'.
> + */

Coding style nit: parameters should be written in caps rather than in quotes
and the "*/" should be on the same line as the "." (two spaces inbetween).

> +                   if (outer_regno < 0 ||
> +                       !in_hard_reg_set_p (reg_class_contents[aclass],
> +                                           outer_mode, outer_regno))

Another one, sorry: || should be at the start of the line rather than
the end.  Also, indentation should be by tabs as far as possible,
then spaces.

Since Vlad already OK'd this version, I committed it as below.  Thanks
for the patch!

Richard


2014-09-30  David Sherwood  <david.sherwood@arm.com>

	* ira-int.h (ira_allocno): Add "wmode" field.
	* ira-build.c (create_insn_allocnos): Add new "parent" function
	parameter.
	* ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
	that cannot be accessed in wmode.

Index: gcc/ira-int.h
===================================================================
--- gcc/ira-int.h	2014-09-22 08:36:23.613797736 +0100
+++ gcc/ira-int.h	2014-09-30 08:50:55.936083472 +0100
@@ -283,6 +283,9 @@ struct ira_allocno
   /* Mode of the allocno which is the mode of the corresponding
      pseudo-register.  */
   ENUM_BITFIELD (machine_mode) mode : 8;
+  /* Widest mode of the allocno which in at least one case could be
+     for paradoxical subregs where wmode > mode.  */
+  ENUM_BITFIELD (machine_mode) wmode : 8;
   /* Register class which should be used for allocation for given
      allocno.  NO_REGS means that we should use memory.  */
   ENUM_BITFIELD (reg_class) aclass : 16;
@@ -315,7 +318,7 @@ struct ira_allocno
      number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
      reload (at this point pseudo-register has only one allocno) which
      did not get stack slot yet.  */
-  short int hard_regno;
+  int hard_regno : 16;
   /* Allocnos with the same regno are linked by the following member.
      Allocnos corresponding to inner loops are first in the list (it
      corresponds to depth-first traverse of the loops).  */
@@ -436,6 +439,7 @@ #define ALLOCNO_TOTAL_NO_STACK_REG_P(A)
 #define ALLOCNO_BAD_SPILL_P(A) ((A)->bad_spill_p)
 #define ALLOCNO_ASSIGNED_P(A) ((A)->assigned_p)
 #define ALLOCNO_MODE(A) ((A)->mode)
+#define ALLOCNO_WMODE(A) ((A)->wmode)
 #define ALLOCNO_PREFS(A) ((A)->allocno_prefs)
 #define ALLOCNO_COPIES(A) ((A)->allocno_copies)
 #define ALLOCNO_HARD_REG_COSTS(A) ((A)->hard_reg_costs)
Index: gcc/ira-build.c
===================================================================
--- gcc/ira-build.c	2014-08-26 12:09:28.234659250 +0100
+++ gcc/ira-build.c	2014-09-30 09:00:05.541337392 +0100
@@ -524,6 +524,7 @@ ira_create_allocno (int regno, bool cap_
   ALLOCNO_BAD_SPILL_P (a) = false;
   ALLOCNO_ASSIGNED_P (a) = false;
   ALLOCNO_MODE (a) = (regno < 0 ? VOIDmode : PSEUDO_REGNO_MODE (regno));
+  ALLOCNO_WMODE (a) = ALLOCNO_MODE (a);
   ALLOCNO_PREFS (a) = NULL;
   ALLOCNO_COPIES (a) = NULL;
   ALLOCNO_HARD_REG_COSTS (a) = NULL;
@@ -893,6 +894,7 @@ create_cap_allocno (ira_allocno_t a)
   parent = ALLOCNO_LOOP_TREE_NODE (a)->parent;
   cap = ira_create_allocno (ALLOCNO_REGNO (a), true, parent);
   ALLOCNO_MODE (cap) = ALLOCNO_MODE (a);
+  ALLOCNO_WMODE (cap) = ALLOCNO_WMODE (a);
   aclass = ALLOCNO_CLASS (a);
   ira_set_allocno_class (cap, aclass);
   ira_create_allocno_objects (cap);
@@ -1859,9 +1861,9 @@ ira_traverse_loop_tree (bool bb_p, ira_l
 
 /* This recursive function creates allocnos corresponding to
    pseudo-registers containing in X.  True OUTPUT_P means that X is
-   a lvalue.  */
+   an lvalue.  PARENT corresponds to the parent expression of X.  */
 static void
-create_insn_allocnos (rtx x, bool output_p)
+create_insn_allocnos (rtx x, rtx outer, bool output_p)
 {
   int i, j;
   const char *fmt;
@@ -1876,7 +1878,15 @@ create_insn_allocnos (rtx x, bool output
 	  ira_allocno_t a;
 
 	  if ((a = ira_curr_regno_allocno_map[regno]) == NULL)
-	    a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+	    {
+	      a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+	      if (outer != NULL && GET_CODE (outer) == SUBREG)
+		{
+		  enum machine_mode wmode = GET_MODE (outer);
+		  if (GET_MODE_SIZE (wmode) > GET_MODE_SIZE (ALLOCNO_WMODE (a)))
+		    ALLOCNO_WMODE (a) = wmode;
+		}
+	    }
 
 	  ALLOCNO_NREFS (a)++;
 	  ALLOCNO_FREQ (a) += REG_FREQ_FROM_BB (curr_bb);
@@ -1887,25 +1897,25 @@ create_insn_allocnos (rtx x, bool output
     }
   else if (code == SET)
     {
-      create_insn_allocnos (SET_DEST (x), true);
-      create_insn_allocnos (SET_SRC (x), false);
+      create_insn_allocnos (SET_DEST (x), NULL, true);
+      create_insn_allocnos (SET_SRC (x), NULL, false);
       return;
     }
   else if (code == CLOBBER)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
       return;
     }
   else if (code == MEM)
     {
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
   else if (code == PRE_DEC || code == POST_DEC || code == PRE_INC ||
 	   code == POST_INC || code == POST_MODIFY || code == PRE_MODIFY)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
 
@@ -1913,10 +1923,10 @@ create_insn_allocnos (rtx x, bool output
   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
     {
       if (fmt[i] == 'e')
-	create_insn_allocnos (XEXP (x, i), output_p);
+	create_insn_allocnos (XEXP (x, i), x, output_p);
       else if (fmt[i] == 'E')
 	for (j = 0; j < XVECLEN (x, i); j++)
-	  create_insn_allocnos (XVECEXP (x, i, j), output_p);
+	  create_insn_allocnos (XVECEXP (x, i, j), x, output_p);
     }
 }
 
@@ -1935,7 +1945,7 @@ create_bb_allocnos (ira_loop_tree_node_t
   ira_assert (bb != NULL);
   FOR_BB_INSNS_REVERSE (bb, insn)
     if (NONDEBUG_INSN_P (insn))
-      create_insn_allocnos (PATTERN (insn), false);
+      create_insn_allocnos (PATTERN (insn), NULL, false);
   /* It might be a allocno living through from one subloop to
      another.  */
   EXECUTE_IF_SET_IN_REG_SET (df_get_live_in (bb), FIRST_PSEUDO_REGISTER, i, bi)
Index: gcc/ira-conflicts.c
===================================================================
--- gcc/ira-conflicts.c	2014-09-22 08:36:22.597810549 +0100
+++ gcc/ira-conflicts.c	2014-09-30 09:00:34.048986621 +0100
@@ -774,6 +774,27 @@ ira_build_conflicts (void)
 				temp_hard_reg_set);
 	    }
 
+	  /* Now we deal with paradoxical subreg cases where certain registers
+	     cannot be accessed in the widest mode.  */
+	  enum machine_mode outer_mode = ALLOCNO_WMODE (a);
+	  enum machine_mode inner_mode = ALLOCNO_MODE (a);
+	  if (GET_MODE_SIZE (outer_mode) > GET_MODE_SIZE (inner_mode))
+	    {
+	      enum reg_class aclass = ALLOCNO_CLASS (a);
+	      for (int j = ira_class_hard_regs_num[aclass] - 1; j >= 0; --j)
+		{
+		   int inner_regno = ira_class_hard_regs[aclass][j];
+		   int outer_regno = simplify_subreg_regno (inner_regno,
+							    inner_mode, 0,
+							    outer_mode);
+		   if (outer_regno < 0
+		       || !in_hard_reg_set_p (reg_class_contents[aclass],
+					      outer_mode, outer_regno))
+		     SET_HARD_REG_BIT (OBJECT_CONFLICT_HARD_REGS (obj),
+				       inner_regno);
+		}
+	    }
+
 	  if (ALLOCNO_CALLS_CROSSED_NUM (a) != 0)
 	    {
 	      int regno;

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Andreas Schwab]

Richard Sandiford <richard.sandiford@arm.com> writes:

> @@ -315,7 +318,7 @@ struct ira_allocno
>       number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
>       reload (at this point pseudo-register has only one allocno) which
>       did not get stack slot yet.  */
> -  short int hard_regno;
> +  int hard_regno : 16;

If you want negative numbers you need to make that explicitly signed.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Richard Sandiford]

Andreas Schwab <schwab@suse.de> writes:
> Richard Sandiford <richard.sandiford@arm.com> writes:
>
>> @@ -315,7 +318,7 @@ struct ira_allocno
>>       number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
>>       reload (at this point pseudo-register has only one allocno) which
>>       did not get stack slot yet.  */
>> -  short int hard_regno;
>> +  int hard_regno : 16;
>
> If you want negative numbers you need to make that explicitly signed.

Are you sure?  In:

  struct { int i : 16; unsigned int j : 1; } x = { -1, 0 };
  int foo (void) { return x.i; }

foo returns -1 rather than 65535.  I can't see any precedent in gcc/*.[hc]
for explicitly marking bitfields as signed.

Thanks,
Richard

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Andreas Schwab]

Richard Sandiford <richard.sandiford@arm.com> writes:

> Andreas Schwab <schwab@suse.de> writes:
>> Richard Sandiford <richard.sandiford@arm.com> writes:
>>
>>> @@ -315,7 +318,7 @@ struct ira_allocno
>>>       number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
>>>       reload (at this point pseudo-register has only one allocno) which
>>>       did not get stack slot yet.  */
>>> -  short int hard_regno;
>>> +  int hard_regno : 16;
>>
>> If you want negative numbers you need to make that explicitly signed.
>
> Are you sure?

See C11, 6.7.2#5.

    Each of the comma-separated multisets designates the same type,
    except that for bit-fields, it is implementation-defined whether the
    specifier int designates the same type as signed int or the same
    type as unsigned int.


Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Richard Earnshaw]

On 30/09/14 12:51, Andreas Schwab wrote:
> Richard Sandiford <richard.sandiford@arm.com> writes:
> 
>> Andreas Schwab <schwab@suse.de> writes:
>>> Richard Sandiford <richard.sandiford@arm.com> writes:
>>>
>>>> @@ -315,7 +318,7 @@ struct ira_allocno
>>>>       number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
>>>>       reload (at this point pseudo-register has only one allocno) which
>>>>       did not get stack slot yet.  */
>>>> -  short int hard_regno;
>>>> +  int hard_regno : 16;
>>>
>>> If you want negative numbers you need to make that explicitly signed.
>>
>> Are you sure?
> 
> See C11, 6.7.2#5.
> 
>     Each of the comma-separated multisets designates the same type,
>     except that for bit-fields, it is implementation-defined whether the
>     specifier int designates the same type as signed int or the same
>     type as unsigned int.
> 
> 
> Andreas.
> 

GCC is written in C++ these days, so technically, you need the C++
standard :-)

GNU C defaults to signed bitfields (see trouble.texi).  However, since
GCC is supposed to bootstrap using a portable ISO C++ compiler, there's
an argument for removing the ambiguity entirely by being explicit.  We
no-longer have to worry about compilers that don't support the signed
keyword.

R.

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Joseph S. Myers]

On Tue, 30 Sep 2014, Richard Earnshaw wrote:

> GCC is written in C++ these days, so technically, you need the C++
> standard :-)

And, while C++14 requires plain int bit-fields to be signed, GCC is 
written in C++98/C++03.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Mike Stump]

On Sep 30, 2014, at 9:15 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Tue, 30 Sep 2014, Richard Earnshaw wrote:
> 
>> GCC is written in C++ these days, so technically, you need the C++
>> standard :-)
> 
> And, while C++14 requires plain int bit-fields to be signed, GCC is 
> written in C++98/C++03.

So, seemingly left unstated in the thread is what is required by the language standard we write in…  From c++98:

  It is implementa-
  tion-defined  whether  bit-fields  and objects of char type are repre-
  sented as signed or unsigned quantities.  The signed specifier  forces
  char  objects  and bit-fields to be signed; it is redundant with other
  integral types.

So, I think you need a signed on bitfields if your want them to be signed.   It doesn’t matter what g++ does, if we want to be portable to any C++ compiler.

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Andreas Schwab]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> And, while C++14 requires plain int bit-fields to be signed,

Thanks, noted for the future.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

RE: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[David Sherwood]

Hi Andreas,

OK, I will fix this.

Thanks,
David Sherwood.

-----Original Message-----
From: Andreas Schwab [mailto:schwab@suse.de] 
Sent: 01 October 2014 08:27
To: Joseph S. Myers
Cc: Richard Earnshaw; David Sherwood; gcc-patches@gcc.gnu.org; vmakarov@redhat.com; Richard
Sandiford
Subject: Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
compiler error)"

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> And, while C++14 requires plain int bit-fields to be signed,

Thanks, noted for the future.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Richard Earnshaw]

On 30/09/14 20:33, Mike Stump wrote:
> On Sep 30, 2014, at 9:15 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> On Tue, 30 Sep 2014, Richard Earnshaw wrote:
>>
>>> GCC is written in C++ these days, so technically, you need the C++
>>> standard :-)
>>
>> And, while C++14 requires plain int bit-fields to be signed, GCC is 
>> written in C++98/C++03.
> 
> So, seemingly left unstated in the thread is what is required by the language standard we write in…  From c++98:
> 

Isn't that exactly what I suggested?

"However, since
GCC is supposed to bootstrap using a portable ISO C++ compiler, there's
an argument for removing the ambiguity entirely by being explicit."


>   It is implementa-
>   tion-defined  whether  bit-fields  and objects of char type are repre-
>   sented as signed or unsigned quantities.  The signed specifier  forces
>   char  objects  and bit-fields to be signed; it is redundant with other
>   integral types.
> 
> So, I think you need a signed on bitfields if your want them to be signed.   It doesn’t matter what g++ does, if we want to be portable to any C++ compiler.
> 

R.

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Mike Stump]

On Oct 1, 2014, at 1:50 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
> Isn't that exactly what I suggested?
> 
> "However, since
> GCC is supposed to bootstrap using a portable ISO C++ compiler, there's
> an argument for removing the ambiguity entirely by being explicit.”

I think one can read that and not understand that in the language standard, it is a requirement.  While those that know C++ well, know that, for those that don’t, I think it helps to see why it is a requirement.

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Richard Sandiford]

"David Sherwood" <david.sherwood@arm.com> writes:
> Hi Andreas,
>
> OK, I will fix this.

I installed David's patch below as obvious.  Tested on x86_64-linux-gnu.

Thanks,
Richard


gcc/
2014-10-03  David Sherwood  <david.sherwood@arm.com>

	* ira-int.h (ira_allocno): Mark hard_regno as signed.

Index: gcc/ira-int.h
===================================================================
--- gcc/ira-int.h	2014-09-30 09:04:23.602162410 +0100
+++ gcc/ira-int.h	2014-10-03 11:09:21.986634891 +0100
@@ -318,7 +318,7 @@ struct ira_allocno
      number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
      reload (at this point pseudo-register has only one allocno) which
      did not get stack slot yet.  */
-  int hard_regno : 16;
+  signed int hard_regno : 16;
   /* Allocnos with the same regno are linked by the following member.
      Allocnos corresponding to inner loops are first in the list (it
      corresponds to depth-first traverse of the loops).  */

Re: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[Vladimir Makarov]

On 09/05/2014 10:51 AM, David Sherwood wrote:
> Hi,
>
> I have a potential fix for a gcc testsuite failure for aarch64 in big
> endian, i.e.
>
> FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
> compiler error)
> FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_y_tst.o compile, (internal
> compiler error)
>
> It is caused by the inappropriate choice of hard registers for paradoxical
> sub registers in
> big endian mode, for example if register 0 is chosen for a paradoxical TI
> subreg on big
> endian then we may end up attempting to reference register -1. Similarly, on
> little endian
> we could end up going beyond the upper bounds of the register file too.
>
> My fix involves adding particular constraints in IRA on the choice of
> register once paradoxical 
> sub registers are encountered. However, Richard Sandiford also proposed an
> alternative
> solution that involves not constraining registers in IRA, but rather making
> use of cost analysis
> instead and letting LRA do the work. Not sure what your preference is ....
Richard's proposal could work too but I prefer your solution because it
directly prevents occurring such situations and the most important sets
up real conflicting hard regs which results in more accurate choice of
dynamic allocno class in IRA and as a result improves coloring in IRA.

The patch is ok (we can not use reg_max_ref_width as it is defined only
in reload).

Could you just change the following line

              if (NULL != parent && GET_CODE (parent) == SUBREG)

 to

              if (parent != NULL && GET_CODE (parent) == SUBREG)

It is very unusual to see such code in GCC.

I see a missed blank too after the comma.  As I remeber we need a blank
in such case according to GNU coding standard.

outer_mode,outer_regno


I'd also use name 'outer' instead of 'parent' as it usual naming for
this.  But it is just a matter of taste.
 
Also as it is your first patch, please test it thoroughly (bootstrap and
check-gcc with comparison of results before and after the patch) on
major platforms available to you (at least x86/x86-64).

If it is ok, you can commit the patch.

Thanks for working on the failure.

> Fix was tested on aarch64 on little and big endian with no regressions.
>
> Regards,
> David Sherwood.
>
> 2014-08-26  David Sherwood  <david.sherwood@arm.com>
>
>         * ira-int.h (ira_allocno): Add "wmode" field.
>         * ira-build.c (create_insn_allocnos): Add new "parent" function
>         parameter.
>         * ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
>         that cannot be accessed in wmode.

RE: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[David Sherwood]

Hi Vladimir,

Sorry, I forgot to CC you on this as it's your code. It's my first attempt at
submitting patches to gcc so I'm still learning as I go!

Kind Regards,
David Sherwood.

-----Original Message-----
From: David Sherwood [mailto:david.sherwood@arm.com] 
Sent: 05 September 2014 15:52
To: 'gcc-patches@gcc.gnu.org'
Subject: Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
compiler error)"

Hi,

I have a potential fix for a gcc testsuite failure for aarch64 in big endian, i.e.

FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)
FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_y_tst.o compile, (internal compiler error)

It is caused by the inappropriate choice of hard registers for paradoxical sub registers in
big endian mode, for example if register 0 is chosen for a paradoxical TI subreg on big
endian then we may end up attempting to reference register -1. Similarly, on little endian
we could end up going beyond the upper bounds of the register file too.

My fix involves adding particular constraints in IRA on the choice of register once paradoxical 
sub registers are encountered. However, Richard Sandiford also proposed an alternative
solution that involves not constraining registers in IRA, but rather making use of cost analysis
instead and letting LRA do the work. Not sure what your preference is ....

Fix was tested on aarch64 on little and big endian with no regressions.

Regards,
David Sherwood.

2014-08-26  David Sherwood  <david.sherwood@arm.com>

        * ira-int.h (ira_allocno): Add "wmode" field.
        * ira-build.c (create_insn_allocnos): Add new "parent" function
        parameter.
        * ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
        that cannot be accessed in wmode.

Fix for "FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal compiler error)"

[David Sherwood]

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

Hi,

I have a potential fix for a gcc testsuite failure for aarch64 in big
endian, i.e.

FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_x_tst.o compile, (internal
compiler error)
FAIL: tmpdir-gcc.dg-struct-layout-1/t028 c_compat_y_tst.o compile, (internal
compiler error)

It is caused by the inappropriate choice of hard registers for paradoxical
sub registers in
big endian mode, for example if register 0 is chosen for a paradoxical TI
subreg on big
endian then we may end up attempting to reference register -1. Similarly, on
little endian
we could end up going beyond the upper bounds of the register file too.

My fix involves adding particular constraints in IRA on the choice of
register once paradoxical 
sub registers are encountered. However, Richard Sandiford also proposed an
alternative
solution that involves not constraining registers in IRA, but rather making
use of cost analysis
instead and letting LRA do the work. Not sure what your preference is ....

Fix was tested on aarch64 on little and big endian with no regressions.

Regards,
David Sherwood.

2014-08-26  David Sherwood  <david.sherwood@arm.com>

        * ira-int.h (ira_allocno): Add "wmode" field.
        * ira-build.c (create_insn_allocnos): Add new "parent" function
        parameter.
        * ira-conflicts.c (ira_build_conflicts): Add conflicts for registers
        that cannot be accessed in wmode.

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

diff --git a/gcc/ira-build.c b/gcc/ira-build.c
index 3f2ab17..d11f3f0 100644
--- a/gcc/ira-build.c
+++ b/gcc/ira-build.c
@@ -524,6 +524,7 @@ ira_create_allocno (int regno, bool cap_p,
   ALLOCNO_BAD_SPILL_P (a) = false;
   ALLOCNO_ASSIGNED_P (a) = false;
   ALLOCNO_MODE (a) = (regno < 0 ? VOIDmode : PSEUDO_REGNO_MODE (regno));
+  ALLOCNO_WMODE (a) = ALLOCNO_MODE (a);
   ALLOCNO_PREFS (a) = NULL;
   ALLOCNO_COPIES (a) = NULL;
   ALLOCNO_HARD_REG_COSTS (a) = NULL;
@@ -893,6 +894,7 @@ create_cap_allocno (ira_allocno_t a)
   parent = ALLOCNO_LOOP_TREE_NODE (a)->parent;
   cap = ira_create_allocno (ALLOCNO_REGNO (a), true, parent);
   ALLOCNO_MODE (cap) = ALLOCNO_MODE (a);
+  ALLOCNO_WMODE (cap) = ALLOCNO_WMODE (a);
   aclass = ALLOCNO_CLASS (a);
   ira_set_allocno_class (cap, aclass);
   ira_create_allocno_objects (cap);
@@ -1859,9 +1861,11 @@ static basic_block curr_bb;
 
 /* This recursive function creates allocnos corresponding to
    pseudo-registers containing in X.  True OUTPUT_P means that X is
-   a lvalue.  */
+   a lvalue.  The 'parent' parameter corresponds to the parent expression
+   of 'rtx'.
+ */
 static void
-create_insn_allocnos (rtx x, bool output_p)
+create_insn_allocnos (rtx x, rtx parent, bool output_p)
 {
   int i, j;
   const char *fmt;
@@ -1876,7 +1880,15 @@ create_insn_allocnos (rtx x, bool output_p)
 	  ira_allocno_t a;
 
 	  if ((a = ira_curr_regno_allocno_map[regno]) == NULL)
-	    a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+            {
+              a = ira_create_allocno (regno, false, ira_curr_loop_tree_node);
+              if (NULL != parent && GET_CODE (parent) == SUBREG)
+                {
+                  enum machine_mode wmode = GET_MODE (parent);
+                  if (GET_MODE_SIZE (wmode) > GET_MODE_SIZE (ALLOCNO_WMODE (a)))
+                    ALLOCNO_WMODE (a) = wmode;
+                }
+            }
 
 	  ALLOCNO_NREFS (a)++;
 	  ALLOCNO_FREQ (a) += REG_FREQ_FROM_BB (curr_bb);
@@ -1887,25 +1899,25 @@ create_insn_allocnos (rtx x, bool output_p)
     }
   else if (code == SET)
     {
-      create_insn_allocnos (SET_DEST (x), true);
-      create_insn_allocnos (SET_SRC (x), false);
+      create_insn_allocnos (SET_DEST (x), NULL, true);
+      create_insn_allocnos (SET_SRC (x), NULL, false);
       return;
     }
   else if (code == CLOBBER)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
       return;
     }
   else if (code == MEM)
     {
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
   else if (code == PRE_DEC || code == POST_DEC || code == PRE_INC ||
 	   code == POST_INC || code == POST_MODIFY || code == PRE_MODIFY)
     {
-      create_insn_allocnos (XEXP (x, 0), true);
-      create_insn_allocnos (XEXP (x, 0), false);
+      create_insn_allocnos (XEXP (x, 0), NULL, true);
+      create_insn_allocnos (XEXP (x, 0), NULL, false);
       return;
     }
 
@@ -1913,10 +1925,10 @@ create_insn_allocnos (rtx x, bool output_p)
   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
     {
       if (fmt[i] == 'e')
-	create_insn_allocnos (XEXP (x, i), output_p);
+	create_insn_allocnos (XEXP (x, i), x, output_p);
       else if (fmt[i] == 'E')
 	for (j = 0; j < XVECLEN (x, i); j++)
-	  create_insn_allocnos (XVECEXP (x, i, j), output_p);
+	  create_insn_allocnos (XVECEXP (x, i, j), x, output_p);
     }
 }
 
@@ -1935,7 +1947,7 @@ create_bb_allocnos (ira_loop_tree_node_t bb_node)
   ira_assert (bb != NULL);
   FOR_BB_INSNS_REVERSE (bb, insn)
     if (NONDEBUG_INSN_P (insn))
-      create_insn_allocnos (PATTERN (insn), false);
+      create_insn_allocnos (PATTERN (insn), NULL, false);
   /* It might be a allocno living through from one subloop to
      another.  */
   EXECUTE_IF_SET_IN_REG_SET (df_get_live_in (bb), FIRST_PSEUDO_REGISTER, i, bi)
diff --git a/gcc/ira-conflicts.c b/gcc/ira-conflicts.c
index 011a865..79e65a9 100644
--- a/gcc/ira-conflicts.c
+++ b/gcc/ira-conflicts.c
@@ -774,6 +774,28 @@ ira_build_conflicts (void)
 				temp_hard_reg_set);
 	    }
 
+          /* Now we deal with paradoxical subreg cases where certain registers
+             cannot be accessed in the widest mode.  */
+          enum machine_mode outer_mode = ALLOCNO_WMODE (a);
+          enum machine_mode inner_mode = ALLOCNO_MODE (a);
+          if (GET_MODE_SIZE (outer_mode) > GET_MODE_SIZE (inner_mode))
+            {
+              int offset = byte_lowpart_offset (outer_mode, inner_mode);
+              enum reg_class aclass = ALLOCNO_CLASS (a);
+              for (int j = ira_class_hard_regs_num[aclass] - 1; j >= 0; --j)
+                {
+                   int inner_regno = ira_class_hard_regs[aclass][j];
+                   int outer_regno = simplify_subreg_regno (inner_regno,
+                                                            inner_mode, 0,
+                                                            outer_mode);
+                   if (outer_regno < 0 ||
+                       !in_hard_reg_set_p (reg_class_contents[aclass],
+                                           outer_mode,outer_regno))
+                     SET_HARD_REG_BIT (OBJECT_CONFLICT_HARD_REGS (obj),
+                                       inner_regno);
+                }
+            }
+
 	  if (ALLOCNO_CALLS_CROSSED_NUM (a) != 0)
 	    {
 	      int regno;
diff --git a/gcc/ira-int.h b/gcc/ira-int.h
index 3d1a1d3..bf1b7e1 100644
--- a/gcc/ira-int.h
+++ b/gcc/ira-int.h
@@ -280,6 +280,9 @@ struct ira_allocno
   /* Mode of the allocno which is the mode of the corresponding
      pseudo-register.  */
   ENUM_BITFIELD (machine_mode) mode : 8;
+  /* Widest mode of the allocno which in at least one case could be
+     for paradoxical subregs where wmode > mode.  */
+  ENUM_BITFIELD (machine_mode) wmode : 8;
   /* Register class which should be used for allocation for given
      allocno.  NO_REGS means that we should use memory.  */
   ENUM_BITFIELD (reg_class) aclass : 16;
@@ -312,7 +315,7 @@ struct ira_allocno
      number (0, ...) - 2.  Value -1 is used for allocnos spilled by the
      reload (at this point pseudo-register has only one allocno) which
      did not get stack slot yet.  */
-  short int hard_regno;
+  int hard_regno : 16;
   /* Allocnos with the same regno are linked by the following member.
      Allocnos corresponding to inner loops are first in the list (it
      corresponds to depth-first traverse of the loops).  */
@@ -433,6 +436,7 @@ struct ira_allocno
 #define ALLOCNO_BAD_SPILL_P(A) ((A)->bad_spill_p)
 #define ALLOCNO_ASSIGNED_P(A) ((A)->assigned_p)
 #define ALLOCNO_MODE(A) ((A)->mode)
+#define ALLOCNO_WMODE(A) ((A)->wmode)
 #define ALLOCNO_PREFS(A) ((A)->allocno_prefs)
 #define ALLOCNO_COPIES(A) ((A)->allocno_copies)
 #define ALLOCNO_HARD_REG_COSTS(A) ((A)->hard_reg_costs)