diff mbox

Fix CSE bogus RTL creation

Message ID 4C17496C.4070908@gnu.org
State New
Headers show

Commit Message

Paolo Bonzini June 15, 2010, 9:35 a.m. UTC 
On 06/15/2010 07:16 AM, Steven Bosscher wrote:
> On Tue, Jun 15, 2010 at 1:41 AM, Mark Mitchell<mark@codesourcery.com>  wrote:
>> Paul Brook wrote:
>>
>>> 2010-06-14  Paul Brook<paul@codesourcery.com>
>>>
>>>        gcc/
>>>        * cse.c (cse_process_notes_1): Call simplify_rtx is a substitution
>>>        was made.
>>
>> I think this qualifies as near-obvious.  OK.
>
> How is this obvious?
>
> 1. It means validate_change validates something that is not canonical RTL.

But that is not the task of validate_change.  It would be done with a 
patch like the attached (which is not meant to be applied right away, 
but would do the same as Paul's in a better way).

> 2. You are canonicalizing something that is in a REG-note. How does
> this non-canonical note content escape from the note into an INSN? Is
> that not the point where this bug should be fixed?
>
> It makes me suspect that this change only papers over a bug elsewhere,
> where a note replacement in an insn is not validated properly.

It may still cause a missed optimization, so it's sensible to apply my 
patch or something like that after investigating (2).  But I also would 
like to understand where is the note used in an insn, and why recog 
isn't trapping it there.

Paolo

Comments

Steven Bosscher June 15, 2010, 10:07 a.m. UTC | #1 
On Tue, Jun 15, 2010 at 11:35 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> 1. It means validate_change validates something that is not canonical RTL.
>
> But that is not the task of validate_change.  It would be done with a patch
> like the attached (which is not meant to be applied right away, but would do
> the same as Paul's in a better way).

That's one possibility. I was thinking, since it's only in a REG_NOTE,
a change doesn't even have to be validated at all (AFAIU, anything
goes in a note), so instead of validate_change the replacement should
perhaps be done with simplify_replace_rtx...?

Ciao!
Steven
Paolo Bonzini June 15, 2010, 11:42 a.m. UTC | #2 
On 06/15/2010 12:07 PM, Steven Bosscher wrote:
> On Tue, Jun 15, 2010 at 11:35 AM, Paolo Bonzini<bonzini@gnu.org>  wrote:
>>> 1. It means validate_change validates something that is not canonical RTL.
>>
>> But that is not the task of validate_change.  It would be done with a patch
>> like the attached (which is not meant to be applied right away, but would do
>> the same as Paul's in a better way).
>
> That's one possibility. I was thinking, since it's only in a REG_NOTE,
> a change doesn't even have to be validated at all (AFAIU, anything
> goes in a note)

Even invalid addresses? (Genuine question; those are definitely ruled 
out by validate_change, and that part dates back to the initial import 
of GCC sources).

> , so instead of validate_change the replacement should
> perhaps be done with simplify_replace_rtx...?

Yes, the whole cse_process_notes function can become a single call to 
simplify_replace_fn_rtx (the replacement function being basically the 
REG case of cse_process_notes_1).  Even better, simplify_replace_fn_rtx 
knows how to deal with SUBREGs, so this case would go away:

         /* We don't substitute VOIDmode constants into these rtx,
            since they would impede folding.  */
         if (GET_MODE (new_rtx) != VOIDmode)
           validate_change (object, &XEXP (x, 0), new_rtx, 0);

Besides possible problems due to invalid addresses (because 
simplify_replace_rtx uses replace_equiv_address_nv) I like this.

Paolo
diff mbox

Patch

Index: cse.c
===================================================================
--- cse.c	(revision 160609)
+++ cse.c	(working copy)
@@ -6005,9 +6005,19 @@  cse_process_notes_1 (rtx x, rtx object, 
       return x;
 
     case MEM:
-      validate_change (x, &XEXP (x, 0),
-		       cse_process_notes (XEXP (x, 0), x, changed), 0);
-      return x;
+      {
+        int n = num_validated_changes ();
+        rtx new = cse_process_notes (XEXP (x, 0), x, changed);
+        validate_change (x, &XEXP (x, 0), new, 1);
+        if (verify_changes (n))
+          {
+            if (!n)
+              confirm_change_group ();
+          }
+        else
+          cancel_changes (n);
+        return x;
+      }
 
     case EXPR_LIST:
     case INSN_LIST:
@@ -6025,7 +6035,7 @@  cse_process_notes_1 (rtx x, rtx object, 
 	/* We don't substitute VOIDmode constants into these rtx,
 	   since they would impede folding.  */
 	if (GET_MODE (new_rtx) != VOIDmode)
-	  validate_change (object, &XEXP (x, 0), new_rtx, 0);
+	  validate_change (object, &XEXP (x, 0), new_rtx, object != NULL_RTX);
 	return x;
       }
 
@@ -6057,8 +6067,11 @@  cse_process_notes_1 (rtx x, rtx object, 
   for (i = 0; i < GET_RTX_LENGTH (code); i++)
     if (fmt[i] == 'e')
       validate_change (object, &XEXP (x, i),
-		       cse_process_notes (XEXP (x, i), object, changed), 0);
+		       cse_process_notes (XEXP (x, i), object, changed), 1);
 
+  canonicalize_change_group (object, x);
+  if (object == NULL_RTX)
+    apply_change_group ();
   return x;
 }