patch to solve PR49936

patch to solve PR49936

[Vladimir Makarov]

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

The following patch makes gcc4.7 behaving as gcc4.6 for the case 
described on http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49936.

The patch was successfully bootstrapped on x86_64 and ppc64.

Committed as rev 177916.

2011-08-19  Vladimir Makarov <vmakarov@redhat.com>

         PR rtl-optimization/49936
         * ira.c (ira_init_register_move_cost): Ignore too small subclasses
         for calculation of max register move costs.


[-- Attachment #2: mips.patch --]
[-- Type: text/plain, Size: 657 bytes --]

Index: ira.c
===================================================================
--- ira.c	(revision 177573)
+++ ira.c	(working copy)
@@ -1501,6 +1501,10 @@ ira_init_register_move_cost (enum machin
 	  sizeof (move_table) * N_REG_CLASSES);
   for (cl1 = 0; cl1 < N_REG_CLASSES; cl1++)
     {
+      /* Some subclasses are to small to have enough registers to hold
+	 a value of MODE.  Just ignore them.  */
+      if (! contains_reg_of_mode[cl1][mode])
+	continue;
       COPY_HARD_REG_SET (temp_hard_regset, reg_class_contents[cl1]);
       AND_COMPL_HARD_REG_SET (temp_hard_regset, no_unit_alloc_regs);
       if (hard_reg_set_empty_p (temp_hard_regset))

Re: patch to solve PR49936

[Richard Sandiford]

Hi Vlad,

Vladimir Makarov <vmakarov@redhat.com> writes:
> The following patch makes gcc4.7 behaving as gcc4.6 for the case 
> described on http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49936.
>
> The patch was successfully bootstrapped on x86_64 and ppc64.
>
> Committed as rev 177916.
>
> 2011-08-19  Vladimir Makarov <vmakarov@redhat.com>
>
>          PR rtl-optimization/49936
>          * ira.c (ira_init_register_move_cost): Ignore too small subclasses
>          for calculation of max register move costs.

Thanks for the patch.  The allocno class costs for MIPS look
much better now.

However, the patch seems to expose a latent problem with the use of
ira_reg_class_max_nregs.  We set the number of allocno objects based
on the ira_reg_class_max_nregs of the allocno class, but often
expect that to be the same as the ira_reg_class_max_nregs of the
pressure class.  I can't see anything in the calculation of the
pressure classes to enforce that though.

In current trunk, this shows up as a failure to build libgcc
on mips64-linux-gnu.  We abort on:

  pclass = ira_pressure_class_translate[ALLOCNO_CLASS (a)];
  nregs = ira_reg_class_max_nregs[pclass][ALLOCNO_MODE (a)];
  gcc_assert (nregs == n);

in ira-lives.c:mark_pseudo_regno_subword_live for the attached
testcase, compiled with -O2 -mabi=64.

In this case it's a MIPS backend bug.  The single pressure class
for MIPS is ALL_REGS, and CLASS_MAX_NREGS (ALL_REGS, TImode)
is returning 4, based on the fact that ALL_REGS includes the
floating-point condition codes.  (CCmode is hard-wired to 4 bytes,
so for CCV2 and CCV4, the correct number of registers is the size
of the mode divided by 4.)  Since floating-point condition codes
can't store TImode, the backend should be ignoring them and
returning 2 instead.  I'm testing a fix for that now.

However, there are other situations where different register banks
really do need different numbers of registers to store the same thing.
E.g. MIPS has a mode in which the core registers are 32 bits but the
floating-point registers are 64 bits.  Thus:

   CLASS_MAX_NREGS (GR_REGS, DFmode) == 2
   CLASS_MAX_NREGS (FP_REGS, DFmode) == 1
   CLASS_MAX_NREGS (ALL_REGS, DFmode) == 2

Moves between GR_REGS and FP_REGS are cheaper than moves between memory
-- MIPS32r2 provides special move instructions -- so the two classes
still end up in the same pressure class.

Richard


typedef int DItype __attribute__((mode(DI)));
typedef int TItype __attribute__((mode(TI)));

DItype
__mulvdi3 (DItype a, DItype b)
{
  const TItype w = (TItype) a * (TItype) b;

  if ((DItype) (w >> (8 * 8)) != (DItype) w >> ((8 * 8) - 1))
    abort ();

  return w;
}

[MIPS, committed] Fix mips_class_max_nregs

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> In this case it's a MIPS backend bug.  The single pressure class
> for MIPS is ALL_REGS, and CLASS_MAX_NREGS (ALL_REGS, TImode)
> is returning 4, based on the fact that ALL_REGS includes the
> floating-point condition codes.  (CCmode is hard-wired to 4 bytes,
> so for CCV2 and CCV4, the correct number of registers is the size
> of the mode divided by 4.)  Since floating-point condition codes
> can't store TImode, the backend should be ignoring them and
> returning 2 instead.  I'm testing a fix for that now.

Here's what I applied after testing mips64-linux-gnu.  As well as fixing
the wrong value for valid combinations, it has the side-effect of
returning an over-the-top value for more invalid combinations than
before.  That's semi- intentional though.  I don't think this macro is
required to detect invalid modes, or return a specific value for them.

Richard


gcc/
	* config/mips/mips.c (mips_class_max_nregs): Check that the mode is
	OK for ST_REGS and FP_REGS before taking those classes into account.

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2011-08-20 19:44:44.000000000 +0100
+++ gcc/config/mips/mips.c	2011-08-20 19:49:06.000000000 +0100
@@ -10630,12 +10630,14 @@ mips_class_max_nregs (enum reg_class rcl
   COPY_HARD_REG_SET (left, reg_class_contents[(int) rclass]);
   if (hard_reg_set_intersect_p (left, reg_class_contents[(int) ST_REGS]))
     {
-      size = MIN (size, 4);
+      if (HARD_REGNO_MODE_OK (ST_REG_FIRST, mode))
+	size = MIN (size, 4);
       AND_COMPL_HARD_REG_SET (left, reg_class_contents[(int) ST_REGS]);
     }
   if (hard_reg_set_intersect_p (left, reg_class_contents[(int) FP_REGS]))
     {
-      size = MIN (size, UNITS_PER_FPREG);
+      if (HARD_REGNO_MODE_OK (FP_REG_FIRST, mode))
+	size = MIN (size, UNITS_PER_FPREG);
       AND_COMPL_HARD_REG_SET (left, reg_class_contents[(int) FP_REGS]);
     }
   if (!hard_reg_set_empty_p (left))

Re: patch to solve PR49936

[Vladimir Makarov]

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

On 08/20/2011 06:13 AM, Richard Sandiford wrote:
> Hi Vlad,
>
> Vladimir Makarov<vmakarov@redhat.com>  writes:
>> The following patch makes gcc4.7 behaving as gcc4.6 for the case
>> described on http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49936.
>>
>> The patch was successfully bootstrapped on x86_64 and ppc64.
>>
>> Committed as rev 177916.
>>
>> 2011-08-19  Vladimir Makarov<vmakarov@redhat.com>
>>
>>           PR rtl-optimization/49936
>>           * ira.c (ira_init_register_move_cost): Ignore too small subclasses
>>           for calculation of max register move costs.
> Thanks for the patch.  The allocno class costs for MIPS look
> much better now.
>
> However, the patch seems to expose a latent problem with the use of
> ira_reg_class_max_nregs.  We set the number of allocno objects based
> on the ira_reg_class_max_nregs of the allocno class, but often
> expect that to be the same as the ira_reg_class_max_nregs of the
> pressure class.  I can't see anything in the calculation of the
> pressure classes to enforce that though.
>
> In current trunk, this shows up as a failure to build libgcc
> on mips64-linux-gnu.  We abort on:
>
>    pclass = ira_pressure_class_translate[ALLOCNO_CLASS (a)];
>    nregs = ira_reg_class_max_nregs[pclass][ALLOCNO_MODE (a)];
>    gcc_assert (nregs == n);
>
> in ira-lives.c:mark_pseudo_regno_subword_live for the attached
> testcase, compiled with -O2 -mabi=64.
>
> In this case it's a MIPS backend bug.  The single pressure class
> for MIPS is ALL_REGS, and CLASS_MAX_NREGS (ALL_REGS, TImode)
> is returning 4, based on the fact that ALL_REGS includes the
> floating-point condition codes.  (CCmode is hard-wired to 4 bytes,
> so for CCV2 and CCV4, the correct number of registers is the size
> of the mode divided by 4.)  Since floating-point condition codes
> can't store TImode, the backend should be ignoring them and
> returning 2 instead.  I'm testing a fix for that now.
Thanks, Richard.  It looks like my merging with Bernd's introduction of 
objects about year ago results in a few typos (they are present in 
mark_pseudo_subword_{live|dead} but absent in other places).  It is 
obvious that allocno class should be used instead of pressure class for 
estimation how many registers are used).  I have the patch to fix it 
too.  You could use it for your patch if you want.

I found also another typo in mark_pseudo_subword_live (strangely it is 
absent mark_pseudo_subword_dead).  The pressure should be increased by 1 
not by nregs.



[-- Attachment #2: mips2.patch --]
[-- Type: text/plain, Size: 1552 bytes --]

Index: ira-lives.c
===================================================================
--- ira-lives.c	(revision 177915)
+++ ira-lives.c	(working copy)
@@ -285,7 +285,7 @@ static void
 mark_pseudo_regno_subword_live (int regno, int subword)
 {
   ira_allocno_t a = ira_curr_regno_allocno_map[regno];
-  int n, nregs;
+  int n;
   enum reg_class pclass;
   ira_object_t obj;
 
@@ -303,14 +303,14 @@ mark_pseudo_regno_subword_live (int regn
     }
 
   pclass = ira_pressure_class_translate[ALLOCNO_CLASS (a)];
-  nregs = ira_reg_class_max_nregs[pclass][ALLOCNO_MODE (a)];
-  gcc_assert (nregs == n);
+  gcc_assert
+    (n == ira_reg_class_max_nregs[ALLOCNO_CLASS (a)][ALLOCNO_MODE (a)]);
   obj = ALLOCNO_OBJECT (a, subword);
 
   if (sparseset_bit_p (objects_live, OBJECT_CONFLICT_ID (obj)))
     return;
 
-  inc_register_pressure (pclass, nregs);
+  inc_register_pressure (pclass, 1);
   make_object_born (obj);
 }
 
@@ -414,7 +414,7 @@ static void
 mark_pseudo_regno_subword_dead (int regno, int subword)
 {
   ira_allocno_t a = ira_curr_regno_allocno_map[regno];
-  int n, nregs;
+  int n;
   enum reg_class cl;
   ira_object_t obj;
 
@@ -430,8 +430,8 @@ mark_pseudo_regno_subword_dead (int regn
     return;
 
   cl = ira_pressure_class_translate[ALLOCNO_CLASS (a)];
-  nregs = ira_reg_class_max_nregs[cl][ALLOCNO_MODE (a)];
-  gcc_assert (nregs == n);
+  gcc_assert
+    (n == ira_reg_class_max_nregs[ALLOCNO_CLASS (a)][ALLOCNO_MODE (a)]);
 
   obj = ALLOCNO_OBJECT (a, subword);
   if (!sparseset_bit_p (objects_live, OBJECT_CONFLICT_ID (obj)))

Re: patch to solve PR49936

[Vladimir Makarov]

On 08/20/2011 06:39 PM, Vladimir Makarov wrote:
> On 08/20/2011 06:13 AM, Richard Sandiford wrote:
>> Hi Vlad,
>>
>> Vladimir Makarov<vmakarov@redhat.com>  writes:
>>> The following patch makes gcc4.7 behaving as gcc4.6 for the case
>>> described on http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49936.
>>>
>>> The patch was successfully bootstrapped on x86_64 and ppc64.
>>>
>>> Committed as rev 177916.
>>>
>>> 2011-08-19  Vladimir Makarov<vmakarov@redhat.com>
>>>
>>>           PR rtl-optimization/49936
>>>           * ira.c (ira_init_register_move_cost): Ignore too small 
>>> subclasses
>>>           for calculation of max register move costs.
>> Thanks for the patch.  The allocno class costs for MIPS look
>> much better now.
>>
>> However, the patch seems to expose a latent problem with the use of
>> ira_reg_class_max_nregs.  We set the number of allocno objects based
>> on the ira_reg_class_max_nregs of the allocno class, but often
>> expect that to be the same as the ira_reg_class_max_nregs of the
>> pressure class.  I can't see anything in the calculation of the
>> pressure classes to enforce that though.
>>
>> In current trunk, this shows up as a failure to build libgcc
>> on mips64-linux-gnu.  We abort on:
>>
>>    pclass = ira_pressure_class_translate[ALLOCNO_CLASS (a)];
>>    nregs = ira_reg_class_max_nregs[pclass][ALLOCNO_MODE (a)];
>>    gcc_assert (nregs == n);
>>
>> in ira-lives.c:mark_pseudo_regno_subword_live for the attached
>> testcase, compiled with -O2 -mabi=64.
>>
>> In this case it's a MIPS backend bug.  The single pressure class
>> for MIPS is ALL_REGS, and CLASS_MAX_NREGS (ALL_REGS, TImode)
>> is returning 4, based on the fact that ALL_REGS includes the
>> floating-point condition codes.  (CCmode is hard-wired to 4 bytes,
>> so for CCV2 and CCV4, the correct number of registers is the size
>> of the mode divided by 4.)  Since floating-point condition codes
>> can't store TImode, the backend should be ignoring them and
>> returning 2 instead.  I'm testing a fix for that now.
> Thanks, Richard.  It looks like my merging with Bernd's introduction 
> of objects about year ago results in a few typos (they are present in 
> mark_pseudo_subword_{live|dead} but absent in other places).  It is 
> obvious that allocno class should be used instead of pressure class 
> for estimation how many registers are used).  I have the patch to fix 
> it too.  You could use it for your patch if you want.
>
> I found also another typo in mark_pseudo_subword_live (strangely it is 
> absent mark_pseudo_subword_dead).  The pressure should be increased by 
> 1 not by nregs.
>
>
I've just committed the patch as rev. 177939.

I successfully bootstrapped it on x86-64 and ppc64.

2011-08-20  Vladimir Makarov <vmakarov@redhat.com>

         * ira-lives.c (mark_pseudo_regno_subword_live): Use allocno class
         for ira_reg_class_max_nregs.  Increase pressure by 1.
         (mark_pseudo_regno_subword_dead): Use allocno class
         for ira_reg_class_max_nregs.