Patchwork [RFA] patch to fix PR56903

login
register
mail settings
Submitter Vladimir Makarov
Date April 10, 2013, 8:15 p.m.
Message ID <5165C84D.3090608@redhat.com>
Download mbox | patch
Permalink /patch/235466/
State New
Headers show

Comments

Vladimir Makarov - April 10, 2013, 8:15 p.m.
The following patch fixes
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56903

In this test case reload pass gets correct value HARD_REGNO_MODE_OK 
because it can not create pseudos (can_create_pseudo) and this was 
actually used that we are in reload (or after reload).  LRA can create 
pseudos therefore it got the wrong answer.  The patch fixes it.

OK for the trunk?

2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>

         PR tree-optimization/56903
         * config/i386/i386.c (ix86_hard_regno_mode_ok): Add
         lra_in_progress for return.

2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>

         PR tree-optimization/56903
         * gcc.target/i386/pr56903.c: New test.
Uros Bizjak - April 11, 2013, 4:51 p.m.
On Wed, Apr 10, 2013 at 10:15 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:
> The following patch fixes
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56903
>
> In this test case reload pass gets correct value HARD_REGNO_MODE_OK because
> it can not create pseudos (can_create_pseudo) and this was actually used
> that we are in reload (or after reload).  LRA can create pseudos therefore
> it got the wrong answer.  The patch fixes it.

Actually, the problem was with *lea_general_2 that allows QImode
values in non-QImode registers. This is OK, since these registers will
be split to paradoxical SImode registers and handled with lea insn.
Apparently, LRA checks if registers can hold this mode through
ix86_hard_regno_mode_ok, which fails in this particular case. BTW:
there are many insn where QImode can live in non-QImode reg, e.g. the
last alternative of *addqi_1, and similar insn patterns, that are
split to lea after reload.

The proposed patch is OK, although I'd propose to change it a bit with
a more descriptive comment:

       if (!TARGET_PARTIAL_REG_STALL)
        return true;
+      /* LRA checks if the hard register is OK for the given mode.
+        QImode values can live in non-QI regs, so we
+        allow all registers here.  */
+      if (lra_in_progress)
+       return true;
       return !can_create_pseudo_p ();


> 2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>
>
>         PR tree-optimization/56903

This is target issue, so PR target/56903.

>         * config/i386/i386.c (ix86_hard_regno_mode_ok): Add
>         lra_in_progress for return.
>
> 2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>
>
>         PR tree-optimization/56903

And here.

>         * gcc.target/i386/pr56903.c: New test.
>
>
> OK for the trunk?
>

The patch is OK with these changes for mainline and 4.8 branch.

Thanks,
Uros.
Vladimir Makarov - April 12, 2013, 5:16 p.m.
On 13-04-11 12:51 PM, Uros Bizjak wrote:
> On Wed, Apr 10, 2013 at 10:15 PM, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> The following patch fixes
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56903
>>
>> In this test case reload pass gets correct value HARD_REGNO_MODE_OK because
>> it can not create pseudos (can_create_pseudo) and this was actually used
>> that we are in reload (or after reload).  LRA can create pseudos therefore
>> it got the wrong answer.  The patch fixes it.
> Actually, the problem was with *lea_general_2 that allows QImode
> values in non-QImode registers. This is OK, since these registers will
> be split to paradoxical SImode registers and handled with lea insn.
> Apparently, LRA checks if registers can hold this mode through
> ix86_hard_regno_mode_ok, which fails in this particular case. BTW:
> there are many insn where QImode can live in non-QImode reg, e.g. the
> last alternative of *addqi_1, and similar insn patterns, that are
> split to lea after reload.
>
> The proposed patch is OK, although I'd propose to change it a bit with
> a more descriptive comment:
>
>         if (!TARGET_PARTIAL_REG_STALL)
>          return true;
> +      /* LRA checks if the hard register is OK for the given mode.
> +        QImode values can live in non-QI regs, so we
> +        allow all registers here.  */
> +      if (lra_in_progress)
> +       return true;
>         return !can_create_pseudo_p ();
>
>
>> 2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>          PR tree-optimization/56903
> This is target issue, so PR target/56903.
>
>>          * config/i386/i386.c (ix86_hard_regno_mode_ok): Add
>>          lra_in_progress for return.
>>
>> 2013-04-10  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>          PR tree-optimization/56903
> And here.
>
>>          * gcc.target/i386/pr56903.c: New test.
>>
>>
>> OK for the trunk?
>>
> The patch is OK with these changes for mainline and 4.8 branch.
>
>
Thanks.  Committed with your proposals as rev. 197927 (trunk) and 197928 
(4.8 branch).

Patch

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 197679)
+++ config/i386/i386.c	(working copy)
@@ -33976,7 +33976,9 @@  ix86_hard_regno_mode_ok (int regno, enum
 	return true;
       if (!TARGET_PARTIAL_REG_STALL)
 	return true;
-      return !can_create_pseudo_p ();
+      /* LRA can create pseudos but it does not mean that it does not
+	 need to know that the hard register in given mode is OK.  */
+      return lra_in_progress || !can_create_pseudo_p ();
     }
   /* We handle both integer and floats in the general purpose registers.  */
   else if (VALID_INT_MODE_P (mode))
Index: testsuite/gcc.target/i386/pr56903.c
===================================================================
--- testsuite/gcc.target/i386/pr56903.c	(revision 0)
+++ testsuite/gcc.target/i386/pr56903.c	(working copy)
@@ -0,0 +1,18 @@ 
+/* PR rtl-optimization/56903 */
+/* { dg-do compile } */
+/* { dg-options "-Os" } */
+/* { dg-additional-options "-march=pentium3" { target ia32 } } */
+
+int a, *b, c;
+struct S { int s : 1; } *fn1 (void);
+extern int fn3 (void), fn4 (int *);
+
+void
+fn2 (void)
+{
+  int e = fn3 ();
+  char f = c + fn1 ()->s * 4;
+  if (*b && f == e)
+    a = *b;
+  fn4 (b);
+}