Patchwork Always check modes in forward_propagate_and_simplify

login
register
mail settings
Submitter Richard Sandiford
Date Sept. 25, 2011, 5 p.m.
Message ID <87vcsgo8vg.fsf@firetop.home>
Download mbox | patch
Permalink /patch/116306/
State New
Headers show

Comments

Richard Sandiford - Sept. 25, 2011, 5 p.m.
While working on a MIPS16 patch, I came across an ICE on code like:

    (set (reg:DI 64) (unspec:DI BLAH))
    (set (reg:SI PSEUDO) (reg:SI 64))

The problem was that fwprop.c:forward_propagate_and_simplify
tried to replace (reg:SI 64) with (unspec:DI BLAH), i.e. it tried
to replace an SImode value with a DImode one.

The function already checks the modes if both the use and def are
subregs.  I think we should always do that.

There was some discussion a while back about whether we should propagate
hard regs.  But I think that was in the context of propagating a hard-reg
SET_SRC.  In this case, BLAH only mentions pseudos, and I think propagating
for:

    (set (reg:SI 64) (unspec:SI BLAH))
    (set (reg:SI PSEUDO) (reg:SI 64))

should still be allowed.

Tested on x86_64-linux-gnu and mips64-linux-gnu.  Also tested by
making sure that the only .o files to change between the old and new
x86_64-linux-gnu stage2 and stage3 compilers were the checksum files
and fwprop.o itself.  Paolo, does this look OK to you?

Richard


gcc/
	* fwprop.c (forward_propagate_and_simplify): After checking
	reg/subreg combinations, check whether the modes are the same.
Paolo Bonzini - Sept. 25, 2011, 7:28 p.m.
On Sun, Sep 25, 2011 at 19:00, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> There was some discussion a while back about whether we should propagate
> hard regs.  But I think that was in the context of propagating a hard-reg
> SET_SRC.  In this case, BLAH only mentions pseudos, and I think propagating
> for:
>
>    (set (reg:SI 64) (unspec:SI BLAH))
>    (set (reg:SI PSEUDO) (reg:SI 64))
>
> should still be allowed.

Yes, at the very least it doesn't extend the live range of hard reg 64
(quite the opposite, actually).

> Tested on x86_64-linux-gnu and mips64-linux-gnu.  Also tested by
> making sure that the only .o files to change between the old and new
> x86_64-linux-gnu stage2 and stage3 compilers were the checksum files
> and fwprop.o itself.  Paolo, does this look OK to you?

Yes, of course.

Paolo

Patch

Index: gcc/fwprop.c
===================================================================
--- gcc/fwprop.c	2011-09-24 13:52:22.000000000 +0100
+++ gcc/fwprop.c	2011-09-24 13:54:50.000000000 +0100
@@ -1232,21 +1232,24 @@  forward_propagate_and_simplify (df_ref u
 
   /* If def and use are subreg, check if they match.  */
   reg = DF_REF_REG (use);
-  if (GET_CODE (reg) == SUBREG
-      && GET_CODE (SET_DEST (def_set)) == SUBREG
-      && (SUBREG_BYTE (SET_DEST (def_set)) != SUBREG_BYTE (reg)
-	  || GET_MODE (SET_DEST (def_set)) != GET_MODE (reg)))
-    return false;
-
+  if (GET_CODE (reg) == SUBREG && GET_CODE (SET_DEST (def_set)) == SUBREG)
+    {
+      if (SUBREG_BYTE (SET_DEST (def_set)) != SUBREG_BYTE (reg))
+	return false;
+    }
   /* Check if the def had a subreg, but the use has the whole reg.  */
-  if (REG_P (reg) && GET_CODE (SET_DEST (def_set)) == SUBREG)
+  else if (REG_P (reg) && GET_CODE (SET_DEST (def_set)) == SUBREG)
     return false;
-
   /* Check if the use has a subreg, but the def had the whole reg.  Unlike the
      previous case, the optimization is possible and often useful indeed.  */
-  if (GET_CODE (reg) == SUBREG && REG_P (SET_DEST (def_set)))
+  else if (GET_CODE (reg) == SUBREG && REG_P (SET_DEST (def_set)))
     reg = SUBREG_REG (reg);
 
+  /* Make sure that we can treat REG as having the same mode as the
+     source of DEF_SET.  */
+  if (GET_MODE (SET_DEST (def_set)) != GET_MODE (reg))
+    return false;
+
   /* Check if the substitution is valid (last, because it's the most
      expensive check!).  */
   src = SET_SRC (def_set);