This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] [MIPS] Implement Errata for 24K and 24KE


Hi Richard,

Here's a new patch which removes the warnings for .noreorder blocks and moves the check for the 24k errata to isns_between. Does this patch look okay?

Catherine

2009-05-06 Catherine Moore <clm@codesourcery.com>

        * config/tc-mips.c (check_for_24k_errata): Return the number
        nops required instead of emitting them.  Remove warnings for
        noreorder sections.
        (insns_between): Call check_for_24k_errata.
        (md_mips_end): Remove call to check_for_24k_errata.
        (append_insn): Likewise.
        (start_noreorder): Likewise.
        (s_change_sec): Likewise.
        * testsuite/gas/mips/eret.l: Remove.

Index: config/tc-mips.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-mips.c,v
retrieving revision 1.406
diff -p -r1.406 tc-mips.c
*** config/tc-mips.c    22 Apr 2009 11:40:25 -0000      1.406
--- config/tc-mips.c    6 May 2009 22:34:01 -0000
*************** reg_lookup (char **s, unsigned int types
*** 1797,1876 ****

/* Implement the ERET/DERET Errata for MIPS 24k.

-     If an ERET/DERET is encountered in a noreorder block,
-     warn if the ERET/DERET is followed by a branch instruction.
-     Also warn if the ERET/DERET is the last instruction in the
-     noreorder block.
-
      IF an ERET/DERET is in a reorder block and is followed by a
      branch instruction, insert a nop.  */

! static void
! check_for_24k_errata (struct mips_cl_insn *insn, int eret_ndx)
! {
!   bfd_boolean next_insn_is_branch = FALSE;
!
!   /* eret_ndx will be -1 for the last instruction in a section
!      and the ERET/DERET will be in insn, not history.  */
!   if (insn
!       && eret_ndx == -1
!       && (insn->insn_opcode == INSN_ERET
!         || insn->insn_opcode == INSN_DERET)
!       && insn->noreorder_p)
!     {
!       as_warn (_("ERET and DERET must be followed by a NOP on the 24K."));
!       return;
!     }
!
!   if (history[eret_ndx].insn_opcode != INSN_ERET
!       && history[eret_ndx].insn_opcode != INSN_DERET)
!     return;
!
!   if (!insn)
!     {
!       if (history[eret_ndx].noreorder_p)
!       as_warn (_("ERET and DERET must be followed by a NOP on the 24K."));
!       return;
!     }
!
!   next_insn_is_branch = ((insn->insn_opcode == INSN_ERET)
!                        || (insn->insn_opcode == INSN_DERET)
!                        || (insn->insn_mo->pinfo
!                            & (INSN_UNCOND_BRANCH_DELAY
!                               | INSN_COND_BRANCH_DELAY
!                               | INSN_COND_BRANCH_LIKELY)));
!
!   if (next_insn_is_branch && history[eret_ndx].noreorder_p)
!     {
!       as_warn (_("ERET and DERET must be followed by a NOP on the 24K."));
!       return;
!     }
!
!   /* Emit nop if the next instruction is a branch.  */
!   if (next_insn_is_branch)
!     {
!       long nop_where, br_where;
!       struct frag *nop_frag, *br_frag;
!       struct mips_cl_insn br_insn, nop_insn;
!
!       emit_nop ();

!       nop_insn = history[eret_ndx - 1];
!       nop_frag = history[eret_ndx - 1].frag;
!       nop_where = history[eret_ndx - 1].where;

!       br_insn = history[eret_ndx];
!       br_frag = history[eret_ndx].frag;
!       br_where = history[eret_ndx].where;

!       move_insn (&nop_insn, br_frag, br_where);
!       move_insn (&br_insn, nop_frag, nop_where);
!       history[eret_ndx-1] = br_insn;
!       history[eret_ndx] = nop_insn;
!     }
  }

  /* Return TRUE if opcode MO is valid on the currently selected ISA and
     architecture.  If EXPANSIONP is TRUE then this check is done while
     expanding a macro.  Use is_opcode_valid_16 for MIPS16 opcodes.  */
--- 1797,1830 ----

/* Implement the ERET/DERET Errata for MIPS 24k.

      IF an ERET/DERET is in a reorder block and is followed by a
      branch instruction, insert a nop.  */

! static int
! check_for_24k_errata (const struct mips_cl_insn *insn1,
!                     const struct mips_cl_insn *insn2)
!   {

!   if (insn1->noreorder_p
!       || (insn1->insn_opcode != INSN_ERET
!           && insn1->insn_opcode != INSN_DERET))
!     return 0;

!   if (insn1->noreorder_p)
!     return 0;

!   if ((insn2->insn_opcode == INSN_ERET)
!        || (insn2->insn_opcode == INSN_DERET)
!        || (insn2->insn_mo->pinfo
!          & (INSN_UNCOND_BRANCH_DELAY
!             | INSN_COND_BRANCH_DELAY
!             | INSN_COND_BRANCH_LIKELY)))
!     return 1;

!   return 0;
  }

  /* Return TRUE if opcode MO is valid on the currently selected ISA and
     architecture.  If EXPANSIONP is TRUE then this check is done while
     expanding a macro.  Use is_opcode_valid_16 for MIPS16 opcodes.  */
*************** md_begin (void)
*** 2156,2164 ****
  void
  md_mips_end (void)
  {
-   if (mips_fix_24k)
-     check_for_24k_errata ((struct mips_cl_insn *) &history[0], -1);
-
    if (! ECOFF_DEBUGGING)
      md_obj_end ();
  }
--- 2110,2115 ----
*************** insns_between (const struct mips_cl_insn
*** 2573,2578 ****
--- 2524,2532 ----
        && INSN2_USES_REG (EXTRACT_OPERAND (RD, *insn1), MIPS_GR_REG))
      return 2;

+   if (mips_fix_24k)
+     return check_for_24k_errata (insn1, insn2);
+
    /* If working around VR4120 errata, check for combinations that need
       a single intervening instruction.  */
    if (mips_fix_vr4120)
*************** append_insn (struct mips_cl_insn *ip, ex
*** 3390,3400 ****
             insn information.  */
          if (pinfo & INSN_UNCOND_BRANCH_DELAY)
            {
-             /* Check for eret/deret before clearing history.  */
-             if (mips_fix_24k)
-               check_for_24k_errata (
-                       (struct mips_cl_insn *) &history[hndx_24k],
-                       hndx_24k+1);
              mips_no_prev_insn ();
            }
        }
--- 3344,3349 ----
*************** append_insn (struct mips_cl_insn *ip, ex
*** 3415,3424 ****
    else
      insert_into_history (0, 1, ip);

-   if (mips_fix_24k)
-     check_for_24k_errata ((struct mips_cl_insn *) &history[hndx_24k],
-                         hndx_24k+1);
-
    /* We just output an insn, so the next one doesn't have a label.  */
    mips_clear_insn_labels ();
  }
--- 3364,3369 ----
*************** start_noreorder (void)
*** 3505,3512 ****
  static void
  end_noreorder (void)
  {
-   if (mips_fix_24k)
-     check_for_24k_errata (NULL, 0);

    mips_opts.noreorder--;
    if (mips_opts.noreorder == 0 && prev_nop_frag != NULL)
--- 3450,3455 ----
*************** s_change_sec (int sec)
*** 12589,12597 ****

mips_emit_delays ();

-   if (mips_fix_24k)
-     check_for_24k_errata ((struct mips_cl_insn *) &history[0], -1);
-
    switch (sec)
      {
      case 't':
--- 12532,12537 ----
*************** s_change_section (int ignore ATTRIBUTE_U
*** 12650,12658 ****
    if (!IS_ELF)
      return;

-   if (mips_fix_24k)
-     check_for_24k_errata ((struct mips_cl_insn *) &history[0], -1);
-
    section_name = input_line_pointer;
    c = get_symbol_end ();
    if (c)
--- 12590,12595 ----


Richard Sandiford wrote:
Catherine Moore <clm@codesourcery.com> writes:
This patch implements the errata for the 24K and 24KE.  The errata
calls for a NOP to be emitted after an ERET/DERET instruction followed
by a branch.  If the ERET/DERET is in a noreorder section, then a
warning will be issued instead.  This functionality requires the
-mfix-24k option.

In addition, there was an error in the delay slot handling in
append_insn.  The code failed to exclude an ERET or DERET instuction
from being moved into a delay slot.  That failure is also corrected
with this patch.

Does this look okay to install?

Other hazards of this kind are handled by insns_between. Was there a reason why you couldn't use it in this case?

I don't like the idea of warning about an ERET at the end of a
.set noreorder block.  In general, if you have:

   .set noreorder
   ...
   FOO
   ...
   .set reorder
   ...
   BAR

and there is a hazard between FOO and BAR, it is perfectly OK to insert
nops at any point between the ".set reorder" and BAR.  This is what we
do for other hazards, and I think we should be consistent.  Likewise
if you have:

   FOO
   ...
   .set noreorder
   ...
   BAR
   ...
   .set reorder

it is perfectly OK to insert nops between FOO and the ".set noreorder".
(FWIW, using insns_between should give you this for free.)

Also, ".set neorder" has traditionally meant "trust me!".  If we want
to warn about caess where we think the programmer is wrong, I think
we need a consistent story.

Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]