diff mbox

PR rtl-optimization/47925: deleting trivially live instructions

Message ID g4pqqcxheb.fsf@linaro.org
State Superseded
Headers show

Commit Message

Richard Sandiford Feb. 28, 2011, 4:17 p.m. UTC
If we have a sequence like:

    (set (reg A) ...)
    (set (reg A) (mem/v (reg A))) [REG_DEAD A]

then a long-standing bug in delete_trivially_dead_insns will cause it to
delete the first instruction.  count_reg_usage counts out how many times
each register is used in the entire function, but it tries to ignore
uses in self-modifications of the form (set (reg A) (... (reg A) ...)).
The problem is that it is ignoring such uses even if the insn contains
an access to volatile memory.  insn_live_p rightly returns true for
such insns, regardless of register counts, so we end up keeping the
self-modification but deleting the instruction that sets its input.

count_reg_usage is set up to predict when insn_live_p would always
return true regardless of register usage.  It is doing this correctly
for instructions that might throw an exception, and for volatile asms,
but it is failing to do it for other side effects that insn_live_p
detects.  These include volatile MEMs and pre-/post-modifications.

Bootstrapped & regression-tested on x86_64-linux-gnu.  OK to install?

The bug showed up as a wrong-code regression in a modified version of Qt,
but I haven't yet found a testcase that fails with 4.6 but passes with
an older GCC.  Is the patch OK now regardless, or should it wait for 4.7?

Richard


gcc/
	PR rtl-optimization/47925
	* cse.c (count_reg_usage): Don't ignore the SET_DEST of instructions
	with side effects.  Remove the more-specific check for volatile asms.

gcc/testsuite/
	PR rtl-optimization/47925
	* gcc.c-torture/execute/pr47925.c: New test.

Comments

Mike Stump Feb. 28, 2011, 6:20 p.m. UTC | #1
On Feb 28, 2011, at 8:17 AM, Richard Sandiford wrote:
> The bug showed up as a wrong-code regression in a modified version of Qt,
> but I haven't yet found a testcase that fails with 4.6 but passes with
> an older GCC.  Is the patch OK now regardless, or should it wait for 4.7?

I personally hate the volatile bugs...  I think they should be shot on the spot.

SInce the alternate code path was pre-existing and presumably tested, this does make the path a bit safer than having to invent a new code path.
Eric Botcazou Feb. 28, 2011, 11:29 p.m. UTC | #2
> The bug showed up as a wrong-code regression in a modified version of Qt,
> but I haven't yet found a testcase that fails with 4.6 but passes with
> an older GCC.  Is the patch OK now regardless, or should it wait for 4.7?

Fine for 4.6 with me, but I think you also need an ack from a RM.

> 	PR rtl-optimization/47925
> 	* cse.c (count_reg_usage): Don't ignore the SET_DEST of instructions
> 	with side effects.  Remove the more-specific check for volatile asms.

OK if you adjust the head comment of the function.
Richard Biener March 1, 2011, 10:05 a.m. UTC | #3
On Tue, Mar 1, 2011 at 12:29 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> The bug showed up as a wrong-code regression in a modified version of Qt,
>> but I haven't yet found a testcase that fails with 4.6 but passes with
>> an older GCC.  Is the patch OK now regardless, or should it wait for 4.7?
>
> Fine for 4.6 with me, but I think you also need an ack from a RM.

Ok with me.

Richard.

>>       PR rtl-optimization/47925
>>       * cse.c (count_reg_usage): Don't ignore the SET_DEST of instructions
>>       with side effects.  Remove the more-specific check for volatile asms.
>
> OK if you adjust the head comment of the function.
>
> --
> Eric Botcazou
>
diff mbox

Patch

Index: gcc/cse.c
===================================================================
--- gcc/cse.c	2010-10-18 10:53:30.000000000 +0100
+++ gcc/cse.c	2011-02-28 15:22:51.000000000 +0000
@@ -6629,9 +6629,10 @@  count_reg_usage (rtx x, int *counts, rtx
     case CALL_INSN:
     case INSN:
     case JUMP_INSN:
-      /* We expect dest to be NULL_RTX here.  If the insn may trap, mark
-         this fact by setting DEST to pc_rtx.  */
-      if (insn_could_throw_p (x))
+      /* We expect dest to be NULL_RTX here.  If the insn may trap,
+	 or if it cannot be deleted due to side-effects, mark this fact
+	 by setting DEST to pc_rtx.  */
+      if (insn_could_throw_p (x) || side_effects_p (PATTERN (x)))
 	dest = pc_rtx;
       if (code == CALL_INSN)
 	count_reg_usage (CALL_INSN_FUNCTION_USAGE (x), counts, dest, incr);
@@ -6671,10 +6672,6 @@  count_reg_usage (rtx x, int *counts, rtx
       return;
 
     case ASM_OPERANDS:
-      /* If the asm is volatile, then this insn cannot be deleted,
-	 and so the inputs *must* be live.  */
-      if (MEM_VOLATILE_P (x))
-	dest = NULL_RTX;
       /* Iterate over just the inputs, not the constraints as well.  */
       for (i = ASM_OPERANDS_INPUT_LENGTH (x) - 1; i >= 0; i--)
 	count_reg_usage (ASM_OPERANDS_INPUT (x, i), counts, dest, incr);
Index: gcc/testsuite/gcc.c-torture/execute/pr47925.c
===================================================================
--- /dev/null	2011-02-21 12:47:04.267827113 +0000
+++ gcc/testsuite/gcc.c-torture/execute/pr47925.c	2011-02-28 16:05:17.000000000 +0000
@@ -0,0 +1,24 @@ 
+struct s { volatile struct s *next; };
+
+void __attribute__((noinline))
+bar (int ignored, int n)
+{
+  asm volatile ("");
+}
+
+int __attribute__((noinline))
+foo (volatile struct s *ptr, int n)
+{
+  int i;
+
+  bar (0, n);
+  for (i = 0; i < n; i++)
+    ptr = ptr->next;
+}
+
+int main (void)
+{
+  volatile struct s rec = { &rec };
+  foo (&rec, 10);
+  return 0;
+}