Patchwork patch to fix pr55141

login
register
mail settings
Submitter Vladimir Makarov
Date Dec. 7, 2012, 9:08 p.m.
Message ID <50C25AB6.1000807@redhat.com>
Download mbox | patch
Permalink /patch/204608/
State New
Headers show

Comments

Vladimir Makarov - Dec. 7, 2012, 9:08 p.m.
The following patch fixes

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55141

   The patch was successfully bootstrapped on x86/x86-64.

   Committed as rev. 194308.

2012-12-07  Vladimir Makarov  <vmakarov@redhat.com>

         testsuite/gcc.target/i386/pr55141.c
         * lra-constraints.c (lra_constraints): Use biggest mode for
         df_set_regs_ever_live.

2012-12-07  Vladimir Makarov  <vmakarov@redhat.com>

         PR rtl-optimization/55141
         * gcc.target/i386/pr55141.c: New.
Richard Sandiford - Dec. 8, 2012, 10:30 a.m.
Vladimir Makarov <vmakarov@redhat.com> writes:
> Index: lra-constraints.c
> ===================================================================
> --- lra-constraints.c	(revision 194307)
> +++ lra-constraints.c	(working copy)
> @@ -3329,8 +3329,9 @@ lra_constraints (bool first_p)
>  	reg = regno_reg_rtx[i];
>  	if ((hard_regno = lra_get_regno_hard_regno (i)) >= 0)
>  	  {
> -	    int j, nregs = hard_regno_nregs[hard_regno][PSEUDO_REGNO_MODE (i)];
> +	    int j, nregs;
>  
> +	    nregs = hard_regno_nregs[hard_regno][lra_reg_info[i].biggest_mode];
>  	    for (j = 0; j < nregs; j++)
>  	      df_set_regs_ever_live (hard_regno + j, true);

It looks like this loop now iterates over a different mode from the
pseudo register's, but starts at the hard register allocated to the
pseudo.  That doesn't work on big-endian targets, where the "extra"
registers come before hard_regno.  I think you need to use
simplify_subreg_regno (...subreg_lowpart_offset (...)).
I realise we only support little-endian for 4.8, but still.

Maybe it'd be worth having a helper function that provides the range.

Richard
Vladimir Makarov - Dec. 11, 2012, 5:27 p.m.
On 12/08/2012 05:30 AM, Richard Sandiford wrote:
> Vladimir Makarov <vmakarov@redhat.com> writes:
>> Index: lra-constraints.c
>> ===================================================================
>> --- lra-constraints.c	(revision 194307)
>> +++ lra-constraints.c	(working copy)
>> @@ -3329,8 +3329,9 @@ lra_constraints (bool first_p)
>>   	reg = regno_reg_rtx[i];
>>   	if ((hard_regno = lra_get_regno_hard_regno (i)) >= 0)
>>   	  {
>> -	    int j, nregs = hard_regno_nregs[hard_regno][PSEUDO_REGNO_MODE (i)];
>> +	    int j, nregs;
>>   
>> +	    nregs = hard_regno_nregs[hard_regno][lra_reg_info[i].biggest_mode];
>>   	    for (j = 0; j < nregs; j++)
>>   	      df_set_regs_ever_live (hard_regno + j, true);
> It looks like this loop now iterates over a different mode from the
> pseudo register's, but starts at the hard register allocated to the
> pseudo.  That doesn't work on big-endian targets, where the "extra"
> registers come before hard_regno.  I think you need to use
> simplify_subreg_regno (...subreg_lowpart_offset (...)).
> I realise we only support little-endian for 4.8, but still.
Yes, Richard, I am aware about it.  There are a few places in LRA where 
similar code should be fixed for big-endian targets.  I'd like to 
address this problem on lra-branch when I am less busy.
> Maybe it'd be worth having a helper function that provides the range.
>
Richard Sandiford - Dec. 11, 2012, 8 p.m.
Vladimir Makarov <vmakarov@redhat.com> writes:
> On 12/08/2012 05:30 AM, Richard Sandiford wrote:
>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>> Index: lra-constraints.c
>>> ===================================================================
>>> --- lra-constraints.c	(revision 194307)
>>> +++ lra-constraints.c	(working copy)
>>> @@ -3329,8 +3329,9 @@ lra_constraints (bool first_p)
>>>   	reg = regno_reg_rtx[i];
>>>   	if ((hard_regno = lra_get_regno_hard_regno (i)) >= 0)
>>>   	  {
>>> -	    int j, nregs = hard_regno_nregs[hard_regno][PSEUDO_REGNO_MODE (i)];
>>> +	    int j, nregs;
>>>   
>>> +	    nregs = hard_regno_nregs[hard_regno][lra_reg_info[i].biggest_mode];
>>>   	    for (j = 0; j < nregs; j++)
>>>   	      df_set_regs_ever_live (hard_regno + j, true);
>> It looks like this loop now iterates over a different mode from the
>> pseudo register's, but starts at the hard register allocated to the
>> pseudo.  That doesn't work on big-endian targets, where the "extra"
>> registers come before hard_regno.  I think you need to use
>> simplify_subreg_regno (...subreg_lowpart_offset (...)).
>> I realise we only support little-endian for 4.8, but still.
> Yes, Richard, I am aware about it.  There are a few places in LRA where 
> similar code should be fixed for big-endian targets.  I'd like to 
> address this problem on lra-branch when I am less busy.

Well, OK, but in the meantime how about putting asserts like:

  gcc_assert (!BYTES_BIG_ENDIAN)

in the affected places?  I'm just worried that this kind of thing leads
to subtle wrong-code bugs in hard-to-reduce cases, so an assert would
make it easier to track which places need updating.

Richard

Patch

Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 194307)
+++ lra-constraints.c	(working copy)
@@ -3329,8 +3329,9 @@  lra_constraints (bool first_p)
 	reg = regno_reg_rtx[i];
 	if ((hard_regno = lra_get_regno_hard_regno (i)) >= 0)
 	  {
-	    int j, nregs = hard_regno_nregs[hard_regno][PSEUDO_REGNO_MODE (i)];
+	    int j, nregs;
 
+	    nregs = hard_regno_nregs[hard_regno][lra_reg_info[i].biggest_mode];
 	    for (j = 0; j < nregs; j++)
 	      df_set_regs_ever_live (hard_regno + j, true);
 	  }
Index: testsuite/gcc.target/i386/pr55141.c
===================================================================
--- testsuite/gcc.target/i386/pr55141.c	(revision 0)
+++ testsuite/gcc.target/i386/pr55141.c	(working copy)
@@ -0,0 +1,29 @@ 
+/* { dg-do run } */
+/* { dg-options "-O -fno-split-wide-types" } */
+
+typedef struct
+{
+  long int p_x, p_y;
+} Point;
+
+static __attribute__ ((noinline, noclone))
+     void foo (Point p0, Point p1, Point p2, Point p3)
+{
+  if (p0.p_x != 1
+      || p1.p_x != 3
+      || p2.p_x != 5
+      || p3.p_x != 7)
+    __builtin_abort ();
+}
+
+int
+main (int argc, char *argv[])
+{
+  Point p0, p1, p2, p3, p4, p5;
+  p0.p_x = 1;
+  p1.p_x = 3;
+  p2.p_x = 5;
+  p3.p_x = 7;
+  foo (p0, p1, p2, p3);
+  return 0;
+}