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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Hi Catherine,

Thanks for the patch.

Catherine Moore <clm@codesourcery.com> writes:
> ! 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;
>    }

In my previous message, I mentioned that the assembler is free to
(and is expected to) insert nops between two hazardous instructions
unless they're in the same noreorder block.  If they're not in the
same noreorder block, there must be at least one "reorderable point"
(ad-hoc terminology, sorry!) between them.  The assembler can insert
the required nops at any of these "reorderable points".

One upshot of this is that gas ought to insert nops in the
following situations:

	.set	noreorder
1:	eret
	.set	reorder
	b	1b

1:	eret
	.set	noreorder
	b	1b
	.set	reorder

That's why none of the other insns_between code checks noreorder_p.
The idea is that insns_between returns the number of nops that would
normally be required between two instructions.  It's then up to the
caller to decide whether the nops should be inserted (and if so, where).

Which was just a long way of saying that you shouldn't check noreorder_p here.

Also, the interface to insns_between is:

/* Return the number of instructions that must separate INSN1 and INSN2,
   where INSN1 is the earlier instruction.  Return the worst-case value
   for any INSN2 if INSN2 is null.  */

but you're not handling the "INSN2 is null" case.  This causes a
segfault on the above testcase.

Since you're removing the warning, you should also remove the testcase
for it.  It fails after this patch.

> *************** 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)

You should only return here if the return value is 1.  We shouldn't
short-cut the rest of the function when check_for_24k_errata returns 0.

(The idea here is that distributors can use -mfix-A -mfix-B -mfix-C
to get code that runs safely on all of A, B and C.)

FWIW, if it makes things easier, I'd be happy with inlining the 24k
logic in this function, like we do for vr4120.  This makes it easier
to check that all "return 2;"s come before all "return 1"s, etc.
(Which they must.)

The following code is dead after the patch, so should be removed:

  int hndx_24k = 0;
...
		  if (mips_fix_24k)
		    hndx_24k++;
...
	  if (mips_fix_24k)
	    hndx_24k++;

I think it'd be worth adding the testcase above to the testsuite.
Same goes for the ".ent/.end" example from my earlier mail, since that
example is pretty representative of what gcc is likely to do.

Richard