Re: [PATCH] Re: REG_DEAD/REG_EQUIV problem.

["John David Anglin" <dave@hiauly1.hia.nrc.ca>]

> 
> > On Thu, 1 Feb 2001, Alan Modra wrote:
> > 
> > > (insn 17 15 21 (set (reg/v:DI 69)
> > >         (mem/u:DI (lo_sum:DI (reg:DI 70)
> > >                 (unspec:DI[ 
> > >                         (symbol_ref/v/f:DI ("*L$C0001"))
> > >                     ]  0)) 0)) 83 {*pa.md:2368} (insn_list 15 (nil))
> > >     (expr_list:REG_EQUIV (mem/u:DI (lo_sum:DI (reg:DI 70)
> > >                 (unspec:DI[ 
> > >                         (symbol_ref/v/f:DI ("*L$C0001"))
> > >                     ]  0)) 0)
> > >         (expr_list:REG_DEAD (reg:DI 70)
> > >             (nil))))
> > 
> > Whee, maybe I'll be a gcc hacker one day.
> > 
> > This fixes the problem for me.  I've no idea whether it's the ideal
> > solution - it may well be possible and desirable to prevent these
> > conflicting notes being issued.
> > 
> > gcc/ChangeLog
> >	* rtlanal.c (reg_death_note_p): New function.
> > 	(reg_mentioned_dies): New function.
> > 	* rtl.h (reg_mentioned_dies): Declare.
> > 	* reload1.c (reload): Don't set reg_equiv_memory_loc for an insn
> > 	that references memory via a reg that dies.

I am not certain this is totally sufficient.  I think it is possible for this
type of situation to occur:

(set (reg: A) (plus (reg: PIC) (high:SI (symbol_ref:SI ("X")))))

(set (reg: B) (mem/u:SI (lo_sum:SI (reg/f: A) (unspec:SI[(symbol_ref:SI ("X")]))))

(set (reg: C) (mem/u:SI (lo_sum:SI (reg/f: A) (unspec:SI[(symbol_ref:SI ("X")]))))
  (expr_list:REG_DEAD (reg: A) (nil))

...

(set (reg: D) (reg: B))

The last set may be in a different basic block than the one in which B is set.
Reload may try to substitute the MEM for B.  It won't notice that the register
mentioned in the MEM is dead.

It would appear that we need to check whether any registers mentioned in
the MEM are still alive at the point where D is set before substituting.

I have a modified patch which stops the generation of the REG_EQUIV note
in local-alloc (see below).  However, it won't work if the above situation
can occur.

There is also a problem with the above MEM being incorrectly substuted
into the insn at line 2099 of pa.md during reload.  I believe this is because
the Q constraint is relaxed during reload:

/* Subroutine for EXTRA_CONSTRAINT.

   Return 1 iff OP is a pseudo which did not get a hard register and
   we are running the reload pass.  */

#define IS_RELOADING_PSEUDO_P(OP) \
  ((reload_in_progress					\
    && GET_CODE (OP) == REG				\
    && REGNO (OP) >= FIRST_PSEUDO_REGISTER		\
    && reg_renumber [REGNO (OP)] < 0))

/* Optional extra constraints for this machine. Borrowed from sparc.h.

   For the HPPA, `Q' means that this is a memory operand but not a
   symbolic memory operand.  Note that an unassigned pseudo register
   is such a memory operand.  Needed because reload will generate
   these things in insns and then not re-recognize the insns, causing
   constrain_operands to fail.

   `R' is used for scaled indexed addresses.

   `S' is the constant 31.

   `T' is for fp loads and stores.  */
#define EXTRA_CONSTRAINT(OP, C)				\
  ((C) == 'Q' ?						\
   (IS_RELOADING_PSEUDO_P (OP)				\
    || (GET_CODE (OP) == MEM				\
	&& (memory_address_p (GET_MODE (OP), XEXP (OP, 0))\
	    || reload_in_progress)			\
	&& ! symbolic_memory_operand (OP, VOIDmode)	\
        && !(GET_CODE (XEXP (OP, 0)) == PLUS		\
	     && (GET_CODE (XEXP (XEXP (OP, 0), 0)) == MULT\
		 || GET_CODE (XEXP (XEXP (OP, 0), 1)) == MULT))))\
   : ((C) == 'R' ?					\
     (GET_CODE (OP) == MEM				\
      && GET_CODE (XEXP (OP, 0)) == PLUS		\
      && (GET_CODE (XEXP (XEXP (OP, 0), 0)) == MULT	\
	  || GET_CODE (XEXP (XEXP (OP, 0), 1)) == MULT)	\
      && (move_operand (OP, GET_MODE (OP))		\
	  || memory_address_p (GET_MODE (OP), XEXP (OP, 0))\
	  || reload_in_progress))			\


I have modified symbolic_memory_operand to detect pic symbolic references
and would like to get rid of the reload_in_progress stuff in EXTRA_CONSTRAINT.
This seems bogus to me and an attempt to work around some kind of reload
bug.  Maybe Jeff can recall why it is there?

A simple way to avoid the problem is to turn off the unchanging bit in
pic MEMs.  This prevents the REG_EQUIV notes from being generated.  I have
done a complete build with the unchanging bit off.  There are two testsuite
failures that I don't think are present in non PIC builds and checks.  These
are gcc.c-torture/compile/900313-1.c and gcc.c-torture/execute/941014-1.c:

Executing on host: /xxx/gnu/gcc-2.97/objdir/gcc/xgcc -B/xxx/gnu/gcc-2.97/objdir/gcc/ /xxx/gnu/gcc-2.97/gcc/testsuite/gcc.c-torture/compile/900313-1.c  -w  -O0 -c  -fPIC -o /xxx/gnu/gcc-2.97/objdir/gcc/testsuite/900313-1.o    (timeout = -1)
/xxx/gnu/gcc-2.97/gcc/testsuite/gcc.c-torture/compile/900313-1.c: In function `main':
/xxx/gnu/gcc-2.97/gcc/testsuite/gcc.c-torture/compile/900313-1.c:7: Insn does not satisfy its constraints:
(insn 104 55 107 (set (reg:SI 111)
	(reg:SI 1 %r1)) 69 {*pa.md:2099} (nil)
    (nil))
/xxx/gnu/gcc-2.97/gcc/testsuite/gcc.c-torture/compile/900313-1.c:7: confused by earlier errors, bailing out

I will try to figure out why SI 111 didn't get a hard register.

Comments?

Dave
-- 
J. David Anglin                                  dave.anglin@nrc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6605)

2001-02-13  John David Anglin  <dave@hiauly1.hia.nrc.ca>

	* local-alloc.c (update_equiv_regs): Don't change a REG_EQUAL
	note to REG_EQUIV note if the MEM contains a reference to a
	register which dies in the insn.
	* rtlanal.c (reg_death_note_p): New function.
	(reg_mentioned_dies): New function.
	* rtl.h (reg_mentioned_dies): Declare.

--- local-alloc.c.orig	Fri Feb  2 00:54:57 2001
+++ local-alloc.c	Tue Feb 13 17:42:12 2001
@@ -822,6 +822,7 @@
       rtx set;
       rtx dest, src;
       int regno;
+      int mem_is_dead = 0;
 
       if (GET_CODE (insn) == NOTE)
 	{
@@ -956,9 +957,19 @@
       reg_equiv[regno].init_insns
 	= gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
 
+      /* Make sure that a MEM doesn't use a register that dies in
+	 this insn.  */
+      if (GET_CODE (src) == MEM)
+	{
+	  rtx death = find_reg_note (insn, REG_DEAD, NULL_RTX);
+	  if (death)
+	    if (reg_mentioned_dies (src, death))
+	      mem_is_dead = 1;
+	}
+
       /* If this register is known to be equal to a constant, record that
 	 it is always equivalent to the constant.  */
-      if (note && ! rtx_varies_p (XEXP (note, 0), 0))
+      if (note && ! mem_is_dead && ! rtx_varies_p (XEXP (note, 0), 0))
 	PUT_MODE (note, (enum machine_mode) REG_EQUIV);
 
       /* If this insn introduces a "constant" register, decrease the priority
--- rtl.h.orig	Sun Jan  7 13:39:19 2001
+++ rtl.h	Mon Feb 12 15:08:06 2001
@@ -1403,6 +1403,7 @@
 extern int computed_jump_p		PARAMS ((rtx));
 typedef int (*rtx_function)             PARAMS ((rtx *, void *));
 extern int for_each_rtx                 PARAMS ((rtx *, rtx_function, void *));
+extern int reg_mentioned_dies		PARAMS ((rtx, rtx));
 extern rtx regno_use_in			PARAMS ((unsigned int, rtx));
 extern int auto_inc_p			PARAMS ((rtx));
 extern void remove_node_from_expr_list	PARAMS ((rtx, rtx *));
--- rtlanal.c.orig	Thu Feb  8 14:05:50 2001
+++ rtlanal.c	Mon Feb 12 15:08:06 2001
@@ -27,6 +27,7 @@
 
 static void set_of_1		PARAMS ((rtx, rtx, void *));
 static void insn_dependent_p_1	PARAMS ((rtx, rtx, void *));
+static int reg_death_note_p	PARAMS ((rtx *, void *));
 
 /* Forward declarations */
 static int computed_jump_p_1	PARAMS ((rtx));
@@ -2327,6 +2328,34 @@
   return 0;
 }
 
+/* Predicate function for reg_mentioned_dies.  */
+
+static int
+reg_death_note_p (reg, notes)
+     rtx *reg;
+     void *notes;
+{
+  rtx link;
+
+  if (GET_CODE (*reg) != REG)
+    return 0;
+
+  for (link = (rtx) notes; link; link = XEXP (link, 1))
+    if (REG_NOTE_KIND (link) == REG_DEAD && *reg == XEXP (link, 0))
+      return 1;
+  return 0;
+}
+
+/* Return 1 if any register found in INSN has a REG_DEAD note in NOTES.  */
+
+int
+reg_mentioned_dies (insn, notes)
+     rtx insn;
+     rtx notes;
+{
+  return for_each_rtx (&insn, reg_death_note_p, notes);
+}
+
 /* Searches X for any reference to REGNO, returning the rtx of the
    reference found if any.  Otherwise, returns NULL_RTX.  */