Re: PR25514: A problem with note distribution in combine

[Roman Zippel <zippel at linux-m68k dot org>]

Hi,

On Tue, 25 Apr 2006, Richard Sandiford wrote:

> In the testcase below, the else block looks like this before combine:
> 
>     A: (set (reg tmp) (reg node))
>          REG_EQUAL (symbol_ref "global_list")
>          REG_DEAD (reg node)
> 
>     B: (set (reg node) (reg next))
>          REG_DEAD (reg next)
> 
>     C: (set (mem (const (plus (symbol_ref "global_list") (const_int 4))))
>             (mem (plus (reg node) (const_int 4))))
> 
>     D: (set (mem (reg tmp)) (mem (reg node)))
>          REG_DEAD (reg node)
> 
> Combine considers combining A and D, but rightly declines because of the
> assignment to NODE in B.  It then replaces the right hand side of A with
> the REG_EQUAL note and tries again.  This time it succeeds with:
> 
>     D': (set (mem (symbol_ref "global_list"))
>              (mem (reg node)))
> 
> distribute_notes then has to update the live range of NODE that previously
> ended at A.

I integrated that patch into Debian/m68k and so far it breaks firefox and 
xorg-server.  The problem is the following:

(insn 195 194 197 19 (set (reg:SI 71)
        (lshiftrt:SI (reg/v:SI 34 [ mech ])
            (const_int 8 [0x8]))) 253 {lshrsi3} (nil)
    (expr_list:REG_DEAD (reg/v:SI 34 [ mech ])
        (nil)))

(insn 197 195 198 19 (set (reg:SI 72)
        (ashift:SI (reg:SI 73)
            (reg:SI 71))) 226 {ashlsi3} (insn_list:REG_DEP_TRUE 195 (nil))
    (expr_list:REG_DEAD (reg:SI 71)
        (expr_list:REG_EQUAL (ashift:SI (const_int 1 [0x1])
                (reg:SI 71))
            (nil))))

(insn 198 197 199 19 (set (mem/s:QI (plus:SI (plus:SI (reg:SI 37 [ D.13576 ])
                    (reg/v/f:SI 44 [ slot ]))
                (const_int 308 [0x134])) [0 <variable>.mechanismBits S1 A8])
        (ior:QI (mem/s:QI (plus:SI (plus:SI (reg:SI 37 [ D.13576 ])
                        (reg/v/f:SI 44 [ slot ]))
                    (const_int 308 [0x134])) [0 <variable>.mechanismBits S1 A8])
            (subreg:QI (reg:SI 72) 3))) 184 {iorqi3} 
(insn_list:REG_DEP_TRUE 194 (insn_list:REG_DEP_TRUE 197 (nil)))
    (expr_list:REG_DEAD (reg:SI 72)
        (expr_list:REG_DEAD (reg:SI 37 [ D.13576 ])
            (nil))))

gcc tries to combine the REG_EQUAL note from 197 into 198 and succeeds, 
but the REG_DEAD note is now applied to the wrong instruction 197 and thus 
also kills 195.
The correct solution would be to check the used SET_SRC, whether the 
register is mentioned. The patch below does that in a IMO rather crude 
way. I'm wondering whether there isn't a better solution, e.g. it could be 
possible to pass the SET_SRC which was combined with i3 to 
distribute_notes() and if the dead register is mentioned there, it's now 
dead in i3, otherwise it still dies in from_insn. The problem here is now 
I'm not sure about the effect on other distribute_notes() users.

bye, Roman


Index: gcc-4.1/gcc/combine.c
===================================================================
--- gcc-4.1.orig/gcc/combine.c	2006-09-13 12:58:44.000000000 +0200
+++ gcc-4.1/gcc/combine.c	2006-09-13 12:57:55.000000000 +0200
@@ -127,7 +127,7 @@ static int total_attempts, total_merges,
    with the value of a REG_EQUAL note.  This is the insn that has been
    so modified, or null if none.  */
 
-static rtx replaced_rhs_insn;
+static rtx replaced_rhs_insn, replaced_rhs_src;
 
 /* Vector mapping INSN_UIDs to cuids.
    The cuids are like uids but increase monotonically always.
@@ -883,9 +883,11 @@ combine_instructions (rtx f, unsigned in
 		      rtx orig = SET_SRC (set);
 		      SET_SRC (set) = note;
 		      replaced_rhs_insn = temp;
+		      replaced_rhs_src = note;
 		      next = try_combine (insn, temp, NULL_RTX,
 					  &new_direct_jump_p);
-		      replaced_rhs_insn = NULL;
+		      replaced_rhs_insn = NULL_RTX;
+		      replaced_rhs_src = NULL_RTX;
 		      if (next)
 			goto retry;
 		      SET_SRC (set) = orig;
@@ -12243,7 +12245,8 @@ distribute_notes (rtx notes, rtx from_in
 	     In both cases, we must search to see if we can find a previous
 	     use of A and put the death note there.  */
 
-	  if (from_insn && from_insn == replaced_rhs_insn)
+	  if (from_insn && from_insn == replaced_rhs_insn
+	      && !reg_overlap_mentioned_p (XEXP (note, 0), replaced_rhs_src))
 	    tem = from_insn;
 	  else
 	    {