Bug 34456 - MIPS: Flaw in branch delay optimization (patch included).
Summary: MIPS: Flaw in branch delay optimization (patch included).
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.1.1
: P3 normal
Target Milestone: 4.1.3
Assignee: Richard Sandiford
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2007-12-14 02:48 UTC by Kaz Kylheku
Modified: 2007-12-19 10:08 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-12-14 07:44:42


Attachments
Patch for caller used registers in mark_set_resources. (343 bytes, patch)
2007-12-14 02:51 UTC, Kaz Kylheku
Details | Diff
execution testcase (325 bytes, text/plain)
2007-12-14 09:02 UTC, Richard Sandiford
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kaz Kylheku 2007-12-14 02:48:37 UTC
This was discovered in GCC 4.1.1 on MIPS, but the problem is in the current SVN head.

The mark_set_resources function incorrectly calculates the potentially clobbered registers for a CALL_INSN, using an obsolete approach relying on the call_used_regs array.

A potential consequence of this is that a register which should be live is marked dead during the liveness analysis which is done in order to determine whether an instruction move into a delay slot is free of resource conflicts.

Thus, it's possible that an instruction which clobbers a register that is live in the fall-through path is moved into a non-annulled delay slot of a branch instruction. This behavior in fact happened when compiling a C++ application for MIPS (n32). The MIPS GP register ($28) is in call_used_regs, but not in call_really_used_regs. 

The hard reg set regs_invalidated_by_call should be used; it contains the right information compiled from global_regs and call_really_used_regs.

The target of the branch instruction in the failed program is an epilogue which restores registers, the first being the GP. This GP-restoring instruction is moved into the branch delay slot, because in the fall-through-code mark_target_live_regs finds a call instruction ahead of anything which uses the GP, and concludes that GP is a dead register.

The result is that a GOT-based indirect function call is made using the caller's GP, which is different from the caller's.

After this patch, nearly the same code is generated, except that the branch is correctly changed to an annulling one (MIPS ``branch likely'').

--- gcc-4.1.1.orig/gcc/resource.c	2007-12-16 11:59:18.000000000 -0800
+++ gcc-4.1.1/gcc/resource.c	2007-12-19 20:59:02.724735120 -0800
@@ -664,9 +664,8 @@
 	  rtx link;
 
 	  res->cc = res->memory = 1;
-	  for (r = 0; r < FIRST_PSEUDO_REGISTER; r++)
-	    if (call_used_regs[r] || global_regs[r])
-	      SET_HARD_REG_BIT (res->regs, r);
+
+	  IOR_HARD_REG_SET (res->regs, regs_invalidated_by_call);
 
 	  for (link = CALL_INSN_FUNCTION_USAGE (x);
 	       link; link = XEXP (link, 1))
Comment 1 Kaz Kylheku 2007-12-14 02:51:54 UTC
Created attachment 14749 [details]
Patch for caller used registers in mark_set_resources.
Comment 2 Steven Bosscher 2007-12-14 07:28:55 UTC
Richard, looks like one you could foster parent...
Comment 3 David Daney 2007-12-14 07:36:55 UTC
Kaz:  A test case would be useful.
Comment 4 Richard Sandiford 2007-12-14 07:44:41 UTC
"steven at gcc dot gnu dot org" <gcc-bugzilla@gcc.gnu.org> writes:
> ------- Comment #2 from steven at gcc dot gnu dot org  2007-12-14 07:28 -------
> Richard, looks like one you could foster parent...

Yeah.  Two dbr_schedule bugs in one week. ;)

I agree with the analysis and fix FWIW.  I'll test it,
and try to come up with a testcase that fails reliably.

Richard
Comment 5 Richard Sandiford 2007-12-14 09:02:06 UTC
Created attachment 14751 [details]
execution testcase

Here's a testcase that produces wrong code, and should fail at runtime.
I'll try it out on a 64-bit GNU/Linux system later.
Comment 6 Richard Sandiford 2007-12-18 07:40:38 UTC
Subject: Bug 34456

Author: rsandifo
Date: Tue Dec 18 07:40:17 2007
New Revision: 131033

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131033
Log:
gcc/
200x-xx-xx  Kaz Kylheku  <kaz@zeugmasystems.com>

	PR rtl-optimization/34456
	* resource.c (mark_set_resources): Use regs_invalidated_by_call
	rather than call_used_regs and global_regs.

gcc/testsuite/
	PR rtl-optimization/34456
	* gcc.c-torture/execute/pr34456.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr34456.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/resource.c
    trunk/gcc/testsuite/ChangeLog

Comment 7 Richard Sandiford 2007-12-18 07:43:29 UTC
Patch applied to trunk.  I'm now going to test 4.2 and 4.1.
Comment 8 Richard Sandiford 2007-12-19 10:04:45 UTC
Subject: Bug 34456

Author: rsandifo
Date: Wed Dec 19 10:04:28 2007
New Revision: 131057

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131057
Log:
gcc/
200x-xx-xx  Kaz Kylheku  <kaz@zeugmasystems.com>

	PR rtl-optimization/34456
	* resource.c (mark_set_resources): Use regs_invalidated_by_call
	rather than call_used_regs and global_regs.

gcc/testsuite/
	PR rtl-optimization/34456
	* gcc.c-torture/execute/pr34456.c: New test.

Added:
    branches/gcc-4_2-branch/gcc/testsuite/gcc.c-torture/execute/pr34456.c
Modified:
    branches/gcc-4_2-branch/gcc/ChangeLog
    branches/gcc-4_2-branch/gcc/resource.c
    branches/gcc-4_2-branch/gcc/testsuite/ChangeLog

Comment 9 Richard Sandiford 2007-12-19 10:06:57 UTC
Subject: Bug 34456

Author: rsandifo
Date: Wed Dec 19 10:05:47 2007
New Revision: 131058

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=131058
Log:
gcc/
200x-xx-xx  Kaz Kylheku  <kaz@zeugmasystems.com>

	PR rtl-optimization/34456
	* resource.c (mark_set_resources): Use regs_invalidated_by_call
	rather than call_used_regs and global_regs.

gcc/testsuite/
	PR rtl-optimization/34456
	* gcc.c-torture/execute/pr34456.c: New test.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/gcc.c-torture/execute/pr34456.c
Modified:
    branches/gcc-4_1-branch/gcc/ChangeLog
    branches/gcc-4_1-branch/gcc/resource.c
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog

Comment 10 Richard Sandiford 2007-12-19 10:08:15 UTC
Thanks for the patch.  Now applied to 4.1 and 4.2.