strengthen protection against REG_EQUIV/EQUAL on !REG set

Richard Sandiford rdsandiford@googlemail.com
Tue Apr 24 22:06:00 GMT 2012


Olivier Hainque <hainque@adacore.com> writes:
> *** /tmp/rkQ7Ep_emit-rtl.c	2012-04-12 11:19:51.830104512 +0200
> --- gcc/emit-rtl.c	2012-04-11 22:39:11.323103686 +0200
> *************** set_unique_reg_note (rtx insn, enum reg_
> *** 4955,4960 ****
> --- 4955,4975 ----
>         if (GET_CODE (datum) == ASM_OPERANDS)
>   	return NULL_RTX;
>   
> +       /* Don't add REG_EQUAL/REG_EQUIV notes on a single_set destination which
> + 	 isn't a REG or a SUBREG of REG.  Such notes are invalid, could lead
> + 	 to bogus assumptions downstream (e.g. that a (set MEM) couldn't trap),
> + 	 and many callers just don't care checking.  Note that we might have
> + 	 single_set (insn) == 0 here, typically from reload attaching REG_EQUAL
> + 	 to USEs for inheritance processing purposes.  */
> +       {
> + 	rtx set  = single_set (insn);
> + 
> + 	if (set && ! (REG_P (SET_DEST (set))
> + 		      || (GET_CODE (SET_DEST (set)) == SUBREG
> + 			  && REG_P (SUBREG_REG (SET_DEST (set))))))
> + 	  return NULL_RTX;
> +       }
> + 

STRICT_LOW_PART is OK too.

I like the idea, but I think we're in danger of having too many
functions check the set.  Further up set_unique_reg_note we have:

      /* Don't add REG_EQUAL/REG_EQUIV notes if the insn
	 has multiple sets (some callers assume single_set
	 means the insn only has one set, when in fact it
	 means the insn only has one * useful * set).  */
      if (GET_CODE (PATTERN (insn)) == PARALLEL && multiple_sets (insn))
	{
	  gcc_assert (!note);
	  return NULL_RTX;
	}

And set_dst_reg_note has:

  rtx set = single_set (insn);

  if (set && SET_DEST (set) == dst)
    return set_unique_reg_note (insn, kind, datum);

Would be nice to use a single function that knows about the extra
contraints here.  Maybe something like the attached?

I'm deliberately requiring the SET to the first rtx in the PARALLEL.
I'm not entirely happy with the optabs.c code:

  if (! rtx_equal_p (SET_DEST (set), target)
      /* For a STRICT_LOW_PART, the REG_NOTE applies to what is inside it.  */
      && (GET_CODE (SET_DEST (set)) != STRICT_LOW_PART
	  || ! rtx_equal_p (XEXP (SET_DEST (set), 0), target)))
    return 1;

either; the note is always GET_MODE (target), so surely this
should be checking for STRICT_LOW_PART before applying rtx_equal_p?
Maybe set_for_reg_notes should return the reg too, and we'd just
apply rtx_equal_p to that.

Richard


gcc/
	* rtl.h (set_for_reg_notes): Declare.
	* emit-rtl.c (set_for_reg_notes): New function.
	(set_unique_reg_note): Use it.
	* optabs.c (add_equal_note): Likewise.

Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h	2012-04-24 22:55:36.002967164 +0100
+++ gcc/rtl.h	2012-04-24 22:59:34.150966581 +0100
@@ -1879,6 +1879,7 @@ extern enum machine_mode choose_hard_reg
 					       bool);
 
 /* In emit-rtl.c  */
+extern rtx set_for_reg_notes (rtx);
 extern rtx set_unique_reg_note (rtx, enum reg_note, rtx);
 extern rtx set_dst_reg_note (rtx, enum reg_note, rtx, rtx);
 extern void set_insn_deleted (rtx);
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c	2012-04-24 22:55:36.002967164 +0100
+++ gcc/emit-rtl.c	2012-04-24 23:00:13.677966484 +0100
@@ -4944,6 +4944,45 @@ force_next_line_note (void)
   last_location = -1;
 }
 
+/* Notes like REG_EQUAL and REG_EQUIV refer to a set in an instruction.
+   Return the set in INSN that such notes describe, or NULL if the notes
+   have no meaning for INSN.  */
+
+rtx
+set_for_reg_notes (rtx insn)
+{
+  rtx pat, reg;
+
+  if (!INSN_P (insn))
+    return NULL_RTX;
+
+  pat = PATTERN (insn);
+  if (GET_CODE (pat) == PARALLEL)
+    {
+      /* We do not use single_set because that ignores SETs of unused
+	 registers.  REG_EQUAL and REG_EQUIV notes really do require the
+	 PARALLEL to have a single SET.  */
+      if (multiple_sets (insn))
+	return NULL_RTX;
+      pat = XVECEXP (pat, 0, 0);
+    }
+
+  if (GET_CODE (pat) != SET)
+    return NULL_RTX;
+
+  reg = SET_DEST (pat);
+
+  /* Notes apply to the contents of a STRICT_LOW_PART.  */
+  if (GET_CODE (reg) == STRICT_LOW_PART)
+    reg = XEXP (reg, 0);
+
+  /* Check that we have a register.  */
+  if (!(REG_P (reg) || GET_CODE (reg) == SUBREG))
+    return NULL_RTX;
+
+  return pat;
+}
+
 /* Place a note of KIND on insn INSN with DATUM as the datum. If a
    note of this type already exists, remove it first.  */
 
@@ -4956,39 +4995,26 @@ set_unique_reg_note (rtx insn, enum reg_
     {
     case REG_EQUAL:
     case REG_EQUIV:
-      /* Don't add REG_EQUAL/REG_EQUIV notes if the insn
-	 has multiple sets (some callers assume single_set
-	 means the insn only has one set, when in fact it
-	 means the insn only has one * useful * set).  */
-      if (GET_CODE (PATTERN (insn)) == PARALLEL && multiple_sets (insn))
-	{
-	  gcc_assert (!note);
-	  return NULL_RTX;
-	}
+      if (!set_for_reg_notes (insn))
+	return NULL_RTX;
 
       /* Don't add ASM_OPERAND REG_EQUAL/REG_EQUIV notes.
 	 It serves no useful purpose and breaks eliminate_regs.  */
       if (GET_CODE (datum) == ASM_OPERANDS)
 	return NULL_RTX;
-
-      if (note)
-	{
-	  XEXP (note, 0) = datum;
-	  df_notes_rescan (insn);
-	  return note;
-	}
       break;
 
     default:
-      if (note)
-	{
-	  XEXP (note, 0) = datum;
-	  return note;
-	}
       break;
     }
 
-  add_reg_note (insn, kind, datum);
+  if (note)
+    XEXP (note, 0) = datum;
+  else
+    {
+      add_reg_note (insn, kind, datum);
+      note = REG_NOTES (insn);
+    }
 
   switch (kind)
     {
@@ -5000,14 +5026,14 @@ set_unique_reg_note (rtx insn, enum reg_
       break;
     }
 
-  return REG_NOTES (insn);
+  return note;
 }
 
 /* Like set_unique_reg_note, but don't do anything unless INSN sets DST.  */
 rtx
 set_dst_reg_note (rtx insn, enum reg_note kind, rtx datum, rtx dst)
 {
-  rtx set = single_set (insn);
+  rtx set = set_for_reg_notes (insn);
 
   if (set && SET_DEST (set) == dst)
     return set_unique_reg_note (insn, kind, datum);
Index: gcc/optabs.c
===================================================================
--- gcc/optabs.c	2012-04-24 22:55:36.002967164 +0100
+++ gcc/optabs.c	2012-04-24 22:59:34.148966581 +0100
@@ -191,7 +191,7 @@ add_equal_note (rtx insns, rtx target, e
        last_insn = NEXT_INSN (last_insn))
     ;
 
-  set = single_set (last_insn);
+  set = set_for_reg_notes (last_insn);
   if (set == NULL_RTX)
     return 1;
 



More information about the Gcc-patches mailing list