diff mbox series

reorg.c (fill_slots_from_thread): Reinstate code typoed out in "Remove CC0".

Message ID 20210515184132.BF8E5203BE@pchp3.se.axis.com
State New
Headers show
Series reorg.c (fill_slots_from_thread): Reinstate code typoed out in "Remove CC0". | expand

Commit Message

Hans-Peter Nilsson May 15, 2021, 6:41 p.m. UTC 
Committed as obvious.

The typo here, is obviously mistaken removal of lines next
to a line that was validly removed.  Targets affected are
those with a delay-slot *and* defining TARGET_FLAGS_REGNUM.
In-tree, a git-grep says the only ones matching are CRIS,
h8300 and visium.  The code removal has the effect of
wrong-code, not reverting the effect of r11-2814.

I'm "guessing" it was the effect of an incorrect conflict
resolution in preparatory work for the r12-440 /
bd1cd0d0e0fe / "Remove CC0" commit, when rebasing a related
branch, and not testing any of the affected targets.  Either
way, the effect was a btest-gcc.sh state of "regress-1152"
for cris-elf.  FWIW, I wrote the removed code (sans the
validly removed cc0 line), a part of what was committed at
2020-08-24 as 0e6c51de8ec47 / r11-2814.

This commit gets cris-elf test-results back to a sane state
(tested at 0ffdbc85d9a6 / r12-761).

gcc:
	* reorg.c (fill_slots_from_thread): Reinstate code typoed out in
	"Remove CC0".
---
 gcc/reorg.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Segher Boessenkool May 15, 2021, 7:41 p.m. UTC | #1 
Hi!

On Sat, May 15, 2021 at 08:41:32PM +0200, Hans-Peter Nilsson wrote:
> The typo here, is obviously mistaken removal of lines next
> to a line that was validly removed.  Targets affected are
> those with a delay-slot *and* defining TARGET_FLAGS_REGNUM.
> In-tree, a git-grep says the only ones matching are CRIS,
> h8300 and visium.  The code removal has the effect of
> wrong-code, not reverting the effect of r11-2814.

Which explains why I didn't see it (I don't run emulators for those
archs, and no hardware either).

I do build cross-compilers for all archs that are supported by Linux,
and then Linux itself by that.  h8300 built fine, and was identical
before and after the cc0 removal.  This did catch another case where I
deleted too much (I think I just hit "dd" too often there :-) )

I think here it was just caused by my misevaluating the original code:

	 && (!HAVE_cc0 || (! (reg_mentioned_p (cc0_rtx, pat)
			     && (! own_thread || ! sets_cc0_p (pat)))))

which was a bit hard to parse for my feeble mind.  Count the number of
!s, for one thing, I must have taken a wrong turn somewhere.

This is just

	 && !(HAVE_cc0
	      && reg_mentioned_p (cc0_rtx, pat)
	      && !(own_thread && sets_cc0_p (pat)))

which is clearer at least for the cc0 removal :-)

(In general, && is almost always more natural than ||, easier to read;
and breaking it up into multiple statements is better still of course).

> I'm "guessing" it was the effect of an incorrect conflict
> resolution in preparatory work for the r12-440 /
> bd1cd0d0e0fe / "Remove CC0" commit, when rebasing a related
> branch, and not testing any of the affected targets.

It isn't, it was there in my dev branch already (which still is on
gcc.gnu.org if you want to check; last time I rebased the version there
was in December).

> Either
> way, the effect was a btest-gcc.sh state of "regress-1152"
> for cris-elf.  FWIW, I wrote the removed code (sans the
> validly removed cc0 line), a part of what was committed at
> 2020-08-24 as 0e6c51de8ec47 / r11-2814.
> 
> This commit gets cris-elf test-results back to a sane state
> (tested at 0ffdbc85d9a6 / r12-761).
> 
> gcc:
> 	* reorg.c (fill_slots_from_thread): Reinstate code typoed out in
> 	"Remove CC0".

Thank you for fixing it!  And sorry for the mistake.

Cheers,


Segher
diff mbox series

Patch

diff --git a/gcc/reorg.c b/gcc/reorg.c
index 4595f9a541f0..7f06a6f6d092 100644
--- a/gcc/reorg.c
+++ b/gcc/reorg.c
@@ -2375,6 +2375,16 @@  fill_slots_from_thread (rtx_jump_insn *insn, rtx condition,
       if (! insn_references_resource_p (trial, &set, true)
 	  && ! insn_sets_resource_p (trial, filter_flags ? &fset : &set, true)
 	  && ! insn_sets_resource_p (trial, &needed, true)
+	  /* If we're handling sets to the flags register specially, we
+	     only allow an insn into a delay-slot, if it either:
+	     - doesn't set the flags register,
+	     - the "set" of the flags register isn't used (clobbered),
+	     - insns between the delay-slot insn and the trial-insn
+	     as accounted in "set", have not affected the flags register.  */
+	  && (! filter_flags
+	      || ! insn_sets_resource_p (trial, &flags_res, true)
+	      || find_regno_note (trial, REG_UNUSED, targetm.flags_regnum)
+	      || ! TEST_HARD_REG_BIT (set.regs, targetm.flags_regnum))
 	  && ! can_throw_internal (trial))
 	{
 	  rtx_insn *prior_insn;