RE: [PATCH] [MIPS] Fix wrong instruction in the delay slot

[Matthew Fortune <Matthew.Fortune@imgtec.com>]

Robert Suchanek <Robert.Suchanek@imgtec.com>
> IMO, the fix is to recognize the empty basic block that has a code_label
> followed by a barrier (ignoring notes and debug_insns), forbid going
> beyond the barrier if the empty block is found in
> skip_consecutive_labels () and first_active_target_insn ().

I can't approve this but I agree that the delay slot filler should not be
allowed to look past barriers to fill delay slots (especially if moving
a memory operation).

The redundant/no-op branch here is also annoying but the delay slot issue
does seem to be an independent problem. I've seen lots of pointless branches
being generated by GCC that get all the way though to emitting code. Perhaps
this is just one of several reasons for generating either always or never
taken conditional branches. They end up as things like BEQ $4, $4 or
BNE $4, $4 which MIPS R6 converts to 'B' or 'nothing' respectively as I have
not had time to track their origin.

Thanks,
Matthew

> 
> The patch was cross tested on mips-img-linux-gnu and sparc-linux-gnu.
> 
> 
> Regards,
> Robert
> 
> gcc/
> 	* reorg.c (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.
> 
> gcc/testsuite
> 	* gcc.target/mips/builtin-unreachable-bug-1.c: New test.
> ---
>  gcc/reorg.c                                        | 28 +++++++
>  .../gcc.target/mips/builtin-unreachable-bug-1.c    | 90
> ++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/mips/builtin-unreachable-
> bug-1.c
> 
> diff --git a/gcc/reorg.c b/gcc/reorg.c
> index cdaa60c..269f666 100644
> --- a/gcc/reorg.c
> +++ b/gcc/reorg.c
> @@ -145,6 +145,30 @@ along with GCC; see the file COPYING3.  If not see
>     These functions are now only used here in reorg.c, and have
> therefore
>     been moved here to avoid inadvertent misuse elsewhere in the
> compiler.  */
> 
> +/* Return true iff 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.  */
> 
> @@ -159,6 +183,8 @@ skip_consecutive_labels (rtx label_or_return)
>    rtx_insn *label = as_a <rtx_insn *> (label_or_return);
> 
>    for (insn = label; insn != 0 && !INSN_P (insn); insn = NEXT_INSN
> (insn))
> +    if (LABEL_P (insn) && label_with_barrier_p (insn))
> +      break;
>      if (LABEL_P (insn))
>        label = insn;
> 
> @@ -267,6 +293,8 @@ first_active_target_insn (rtx insn)  {
>    if (ANY_RETURN_P (insn))
>      return insn;
> +  if (LABEL_P (insn) && label_with_barrier_p (as_a <rtx_insn *>
> (insn)))
> +    return NULL_RTX;
>    return next_active_insn (as_a <rtx_insn *> (insn));  }
> 
> 
> diff --git a/gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c
> b/gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c
> new file mode 100644
> index 0000000..65eb9a3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/builtin-unreachable-bug-1.c
> @@ -0,0 +1,90 @@
> +/* { dg-options "-mcompact-branches=never -G0 -mno-abicalls" } */
> +/* { dg-final { scan-assembler-not
> +"beq\t\\\$4,\\\$0,\\\$L\[0-9\]+\n\tlw\t\\\$4,%lo\\(mips_cm_is64\\)\\(\\
> +\$4\\)" } } */
> +
> +enum
> +{
> +  CPU_R4700,
> +  CPU_R5000,
> +  CPU_R5500,
> +  CPU_NEVADA,
> +  CPU_RM7000,
> +  CPU_SR71000,
> +  CPU_4KC,
> +  CPU_4KEC,
> +  CPU_4KSC,
> +  CPU_24K,
> +  CPU_34K,
> +  CPU_1004K,
> +  CPU_74K,
> +  CPU_ALCHEMY,
> +  CPU_PR4450,
> +  CPU_BMIPS32,
> +  CPU_BMIPS3300,
> +  CPU_BMIPS5000,
> +  CPU_JZRISC,
> +  CPU_M14KC,
> +  CPU_M14KEC,
> +  CPU_INTERAPTIV,
> +  CPU_P5600,
> +  CPU_PROAPTIV,
> +  CPU_1074K,
> +  CPU_M5150,
> +  CPU_I6400,
> +  CPU_R3000,
> +  CPU_5KC,
> +  CPU_5KE,
> +  CPU_20KC,
> +  CPU_25KF,
> +  CPU_SB1,
> +  CPU_SB1A,
> +  CPU_XLP,
> +  CPU_QEMU_GENERIC
> +};
> +
> +struct cpuinfo_mips
> +{
> +  long options;
> +  int isa_level;
> +} cpu_data[1];
> +
> +struct thread_info
> +{
> +  int cpu;
> +};
> +
> +int a, b, c, d, e, mips_cm_is64;
> +
> +static int __attribute__ ((__cold__))
> +mips_sc_probe_cm3 ()
> +{
> +  struct thread_info *info;
> +  struct cpuinfo_mips *c = &cpu_data[info->cpu];
> +  if (mips_cm_is64)
> +    c->options = 0;
> +  return 1;
> +}
> +
> +int
> +mips_sc_probe ()
> +{
> +  struct cpuinfo_mips ci = cpu_data[c];
> +  if (mips_cm_is64)
> +    __asm__("" ::: "memory");
> +  if (d)
> +    return mips_sc_probe_cm3 ();
> +  if (ci.isa_level)
> +    switch (a)
> +      {
> +      case CPU_QEMU_GENERIC:
> +      case CPU_I6400:
> +      case CPU_R3000:
> +      case CPU_NEVADA:
> +      case CPU_BMIPS3300:
> +	break;
> +      default:
> +	__builtin_unreachable ();
> +      }
> +  switch (a)
> +    case CPU_R3000:
> +      e = b;
> +}
> --
> 2.4.5