diff mbox

strengthen protection against REG_EQUIV/EQUAL on !REG set

Message ID 871unc7pkb.fsf@talisman.home
State New
Headers show

Commit Message

Richard Sandiford April 24, 2012, 10:06 p.m. UTC 
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.

Comments

Olivier Hainque April 26, 2012, 8:39 a.m. UTC | #1 
Hello Richard,

On Apr 25, 2012, at 00:06 , Richard Sandiford wrote:
> STRICT_LOW_PART is OK too.

 Ah, right.

> 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.

 Looks cleaner indeed. Do you want me to test ?

 Thanks a lot for your feedback.

 Olivier
Olivier Hainque May 4, 2012, 1:34 p.m. UTC | #2 
Hello Richard,

Re $subject, at http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01515.html

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

<< 2012-04-24  ...

	* 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.
>>

I had answered:
> Looks cleaner indeed. Do you want me to test ?

I gave it a try. Your patch bootstraps and regtests fine on mainline for x86-linux.

May I commit ?

Thanks in advance for your feedback,

With Kind Regards,

Olivier
Richard Sandiford May 4, 2012, 2:16 p.m. UTC | #3 
Olivier Hainque <hainque@adacore.com> writes:
> Hello Richard,
>
> Re $subject, at http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01515.html
>
> You suggested:
>>> Would be nice to use a single function that knows about the extra
>>> contraints here.  Maybe something like the attached?
>
> << 2012-04-24  ...
>
> 	* 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.
>>>
>
> I had answered:
>> Looks cleaner indeed. Do you want me to test ?
>
> I gave it a try. Your patch bootstraps and regtests fine on mainline for x86-linux.

Sorry, was going to test this earlier, but got distracted by
lower-subreg stuff.  I need to fix the subreg handling so that
we check whether the inner part of a SUBREG is a REG (done in
my copy at home).  I also wanted to make sure there were no
asm differences due to notes being wrongly dropped.

Hope to do that this weekend.

Richard
Olivier Hainque May 4, 2012, 3:43 p.m. UTC | #4 
On May 4, 2012, at 16:16 , Richard Sandiford wrote:

> Sorry, was going to test this earlier, but got distracted by
> lower-subreg stuff.

 No problem at all. I just happened to have had an opportunity to
 test as part of a series of miscellaneous other submissions.

>  I need to fix the subreg handling so that
> we check whether the inner part of a SUBREG is a REG (done in
> my copy at home).

 Indeed. That was in the original patch and I missed the
 difference in the alternate version.

>  I also wanted to make sure there were no
> asm differences due to notes being wrongly dropped.

 Ah, nice :)

 Thanks much for your feedback,

 Olivier
Richard Sandiford May 7, 2012, 9:36 a.m. UTC | #5 
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Olivier Hainque <hainque@adacore.com> writes:
>> Hello Richard,
>>
>> Re $subject, at http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01515.html
>>
>> You suggested:
>>>> Would be nice to use a single function that knows about the extra
>>>> contraints here.  Maybe something like the attached?
>>
>> << 2012-04-24  ...
>>
>> 	* 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.
>>>>
>>
>> I had answered:
>>> Looks cleaner indeed. Do you want me to test ?
>>
>> I gave it a try. Your patch bootstraps and regtests fine on mainline for x86-linux.
>
> Sorry, was going to test this earlier, but got distracted by
> lower-subreg stuff.  I need to fix the subreg handling so that
> we check whether the inner part of a SUBREG is a REG (done in
> my copy at home).  I also wanted to make sure there were no
> asm differences due to notes being wrongly dropped.

So I tried compiling some recent cc1 .ii files on x86_64 at -O2.
The only differences were in fixed-value.ii.  In this case we
used to create:

---------------------------------------------------------------------------
(insn 899 898 900 68 (parallel [
            (set (reg/f:DI 597)
                (plus:DI (reg/f:DI 20 frame)
                    (const_int -32 [0xffffffffffffffe0])))
            (clobber (reg:CC 17 flags))
        ]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 252 {*adddi_1}
     (nil))

(insn 900 899 901 72 (parallel [
            (set (reg:DI 598)
                (plus:DI (reg:DI 597)
                    (const_int 8 [0x8])))
            (clobber (reg:CC 17 flags))
        ]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 -1
     (nil))

(insn 901 900 902 72 (set (mem/f:DI (plus:DI (reg/f:DI 56 virtual-outgoing-args)
                (const_int 24 [0x18])) [0 S8 A64])
        (reg:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 -1
     (expr_list:REG_EQUAL (plus:DI (reg:DI 597)
            (const_int 8 [0x8]))
        (nil)))

[...other uses of 597...]

(insn 921 920 922 73 (parallel [
            (set (reg:DI 604)
                (plus:DI (reg:DI 603)
                    (const_int 8 [0x8])))
            (clobber (reg:CC 17 flags))
        ]) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 -1
     (nil))
---------------------------------------------------------------------------

where 901 has a REG_EQUAL note against a MEM.  cse1 changes the note to:

---------------------------------------------------------------------------
(insn 901 900 902 68 (set (mem/f:DI (plus:DI (reg/f:DI 7 sp)
                (const_int 24 [0x18])) [0 S8 A64])
        (reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:568 62 {*movdi_internal_rex64}
     (expr_list:REG_EQUAL (plus:DI (reg/f:DI 20 frame)
            (const_int -24 [0xffffffffffffffe8]))
        (nil)))
---------------------------------------------------------------------------

but doesn't touch 921 (probably worth finding out why).  cse2 then sees
this note and records it as having the same value as both the MEM and
reg 598.  So when it comes to insn 921 and replaces the source with reg 598,
it also adds a note:

---------------------------------------------------------------------------
(insn 921 1285 939 71 (set (reg/f:DI 698)
        (reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 62 {*movdi_internal_rex64}
     (expr_list:REG_EQUAL (plus:DI (reg/f:DI 20 frame)
            (const_int -24 [0xffffffffffffffe8]))
        (expr_list:REG_UNUSED (reg:CC 17 flags)
            (nil))))
---------------------------------------------------------------------------

Reload is then able to use this information to drop the 698 and
rematerialise it where necessary.  After the patch we just get:

---------------------------------------------------------------------------
(insn 921 1285 939 71 (set (reg/f:DI 698)
        (reg/f:DI 598)) /home/richards/gcc/HEAD/gcc/gcc/fixed-value.c:580 62 {*movdi_internal_rex64}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))
---------------------------------------------------------------------------

The problem here seems to be some inconsistency about what is considered
"constant" in cse.  The condition for table elements is:

  elt->is_const = (CONSTANT_P (x) || fixed_base_plus_p (x));

But the condition for values that been calculated through folding
(e.g. because no note exists) is:

      if (src_const == 0
	  && (CONSTANT_P (src_folded)
	      /* Consider (minus (label_ref L1) (label_ref L2)) as
		 "constant" here so we will record it. This allows us
		 to fold switch statements when an ADDR_DIFF_VEC is used.  */
	      || (GET_CODE (src_folded) == MINUS
		  && GET_CODE (XEXP (src_folded, 0)) == LABEL_REF
		  && GET_CODE (XEXP (src_folded, 1)) == LABEL_REF)))
	src_const = src_folded, src_const_elt = elt;

fixed_base_plus_p has some old code related to rtl inlining,
so these days I think the first condition should be equivalent to:

  elt->is_const = function_invariant_p (x);

I'm not sure why fixed_plus_base_p checks fixed_regs[ARG_POINTER_REGNUM]
though.  (function_invariant_p doesn't.)

If we change the second to also include fixed_base_plus_p/
function_invariant_p:

      if (src_const == 0
	  && (function_invariant_p (src_folded)
	      /* Consider (minus (label_ref L1) (label_ref L2)) as
		 "constant" here so we will record it. This allows us
		 to fold switch statements when an ADDR_DIFF_VEC is used.  */
	      || (GET_CODE (src_folded) == MINUS
		  && GET_CODE (XEXP (src_folded, 0)) == LABEL_REF
		  && GET_CODE (XEXP (src_folded, 1)) == LABEL_REF)))
	src_const = src_folded, src_const_elt = elt;

then we get the note back.  That in itself isn't a complete patch,
but it makes a surprising difference.  I've filed 53260 to record this.

Even though the REG_EQUAL note above is invalid (and documented as
being invalid), I'm guessing people wouldn't want the patch to be
applied until this is sorted out.

Richard
diff mbox

Patch

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;