diff mbox series

[28/61] Fix wrong instruction in the delay slot

Message ID 20250131171232.1018281-30-aleksandar.rakic@htecgroup.com
State New
Headers
Series Improve Mips target |

Commit Message

Aleksandar Rakic Jan. 31, 2025, 5:13 p.m. UTC 
  From: Robert Suchanek <robert.suchanek@imgtec.com>

The problematic test case shows that the use of __builtin_unreachable ()
has a branch not optimised away causing confusion in the eager
delay slot filler if the "unreachable" is moved elsewhere by the block
reordering pass.

It appears that a series of unfortunate events causes a wrong
instruction to be placed in the delay slot:

1. The branch is not optimised away during expansion.  It has a diamond
   shape so the unreachable case falls through.
2. The block reordering pass moves the basic block elsewhere.
3. The eager delay slot filler (EDSF):
   a. It initially skips all the consecutive labels, ignoring barriers,
      until it finds an instruction.  This is done by design.  Similarly
      what first_active_target_insn() does.
   b. The branch now points to a load for the branch taken case.
   c. The arithmetic shift left instruction is not placed in the slot
      because the EDSF detects that there is a conflict with the
      resource usage (because of a set $4, $4 being referenced or both
      but very likely because it's referenced).
   d. As (c) failed, another attempt is taken and the other thread/path
      explored.  This time it succeeds as, at least it appears that,
      the reverse search for the branch taken path looks for the
      beginning of the basic block and it does not see that $4 is also
      used.  The lack of referencing $4 by the shift is likely to be the
      cause of not seeing the usage.
   e. As (d) succeeded, the load is "legitimately" placed in the delay
      slot.

Perhaps this is a vague description but this is more and less what is
happening.

The fix attempts to treat the unreachable block (that represents
__builtin_unreachable) in a special way:

1. The label is not skipped if it is a label with a barrier only.  Notes
   and debug instructions are ignored.  This prevents redirecting the
   jump to a wrong place that seemed to be treated as a valid
   redirection.  Since the behaviour of such branching is undefined, we
   don't want to analyse the taken path.

2. The first_active_target_insn() must recognize the unreachable block
   and not to go beyond the barrier for the same reason as above.

3. With this in place, the eager delay slot filler uses the correct
   instruction.  We don't care where the branch branches to as the
   behaviour of the program is undefined.  The slot is not filled
   letting the assembler to do the right thing (.set noreorder/reorder
   are not emitted).

gcc/
	* reorg.cc (label_with_barrier_p): New function.
	(skip_consecutive_labels): Use it.  Don't skip the label if an
	empty block is found.
	(first_active_target_insn): Likewise.  Don't ignore the empty
	block when searching for the next active instruction.

Cherry-picked 3667d07c7f0512e8996eab9ab75efc79ac1827c2
from https://github.com/MIPS/gcc

Signed-off-by: Robert Suchanek <robert.suchanek@mips.com>
Signed-off-by: Faraz Shahbazker <fshahbazker@wavecomp.com>
Signed-off-by: Aleksandar Rakic <aleksandar.rakic@htecgroup.com>
---
 gcc/reorg.cc | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Eric Botcazou Feb. 1, 2025, 9:34 a.m. UTC | #1 
See PR rtl-optimization/117327 for a similar issue.

> gcc/
> 	* reorg.cc (label_with_barrier_p): New function.
> 	(skip_consecutive_labels): Use it.  Don't skip the label if an
> 	empty block is found.
> 	(first_active_target_insn): Likewise.  Don't ignore the empty
> 	block when searching for the next active instruction.
diff mbox series

Patch

diff --git a/gcc/reorg.cc b/gcc/reorg.cc
index 68bf30801cf..91a752b7d4a 100644
--- a/gcc/reorg.cc
+++ b/gcc/reorg.cc
@@ -113,6 +113,30 @@  along with GCC; see the file COPYING3.  If not see
    These functions are now only used here in reorg.cc, and have therefore
    been moved here to avoid inadvertent misuse elsewhere in the compiler.  */
 
+/* Return true if a LABEL is followed by a BARRIER.  Ignore notes and debug
+   instructions.  */
+
+static bool
+label_with_barrier_p (rtx_insn *label)
+{
+  bool empty_bb = true;
+
+  if (GET_CODE (label) != CODE_LABEL)
+    empty_bb = false;
+  else
+    label = NEXT_INSN (label);
+
+  while (!BARRIER_P (label) && empty_bb)
+  {
+    if (!(DEBUG_INSN_P (label)
+	  || NOTE_P (label)))
+      empty_bb = false;
+    label = NEXT_INSN (label);
+  }
+
+  return empty_bb;
+}
+
 /* Return the last label to mark the same position as LABEL.  Return LABEL
    itself if it is null or any return rtx.  */
 
@@ -140,6 +164,8 @@  skip_consecutive_labels (rtx label_or_return)
   for (insn = label;
        insn != 0 && !INSN_P (insn) && !BARRIER_P (insn);
        insn = NEXT_INSN (insn))
+    if (LABEL_P (insn) && label_with_barrier_p (insn))
+      break;
     if (LABEL_P (insn))
       label = insn;
 
@@ -230,6 +256,8 @@  first_active_target_insn (rtx insn)
 {
   if (ANY_RETURN_P (insn))
     return insn;
+  if (LABEL_P (insn) && label_with_barrier_p ((rtx_insn *)insn))
+    return NULL_RTX;
   return next_active_insn (as_a <rtx_insn *> (insn));
 }