[PATCH v2] Work around the NOP issue of Loongson2F

Richard Sandiford rdsandiford@googlemail.com
Mon Nov 16 20:04:00 GMT 2009


Why did you remove the md_begin stuff that handled the initialisation
of nop_insn?  What I was trying to say in my review was that:

(a) Your code to handle _implicit_ nops (in md_begin) looked good.
    I agree with that approach.

(b) The problem with the previous patch was that it didn't handle
    _explicit_ nops.

The new patch handles explicit nops, but doesn't handle implicit nops.
We need to handle both.  Of course, the code in md_begin will need
to skip the "nop" INSN_MACRO entry, but that should be easy.

Wu Zhangjin <wuzhangjin@gmail.com> writes:
> @@ -7361,6 +7367,17 @@ macro (struct mips_cl_insn *ip)
>        move_register (dreg, sreg);
>        break;
>  
> +    case M_NOP:
> +      if (mips_fix_loongson2f_nop) {
> +	static int inserted = 0;
> +	if (!inserted) {
> +	  hash_jam(op_hash, "nop.at", (void *)&loongson2f_nop_insn);
> +	  inserted = 1;
> +	}
> +        macro_build(NULL, "nop.at", "", 0);
> +      }
> +      break;
> +

Not correctly formatted.  You shouldn't need anything so complex though;
just use:

  append_insn (NOP_INSN, 0, BFD_RELOC_NONE);

> diff --git a/gas/doc/c-mips.texi b/gas/doc/c-mips.texi
> index 9ca7ebf..d1c57dd 100644
> --- a/gas/doc/c-mips.texi
> +++ b/gas/doc/c-mips.texi
> @@ -182,6 +182,11 @@ all problems in hand-written assembler code.
>  @itemx -no-mfix-vr4130
>  Insert nops to work around the VR4130 @samp{mflo}/@samp{mfhi} errata.
>  
> +@item -mfix-loongson2f-nop
> +@itemx -no-mfix-loongson2f-nop
> +Replace nops by @code{or at,at,zero} to work around the Loongson2F @samp{nop}
> +errata.
> +

A bit more explanation would be good, if you can give it.  I wasn't sure
from your earlier description to Maciej what the requirements for the
new nop were.  Why is "at" so special in avoiding the buffer-full condition?

Can you say specifically which revisions of the Loongson 2F are free from
the errata?  It would be good to document that too, if possible.

Neither of these are hard requirements though (as you can tell from the
existing -mfix-* options!).  They're just nice-to-have.

> diff --git a/gas/testsuite/gas/mips/loongson-2f-2.s b/gas/testsuite/gas/mips/loongson-2f-2.s
> new file mode 100644
> index 0000000..c4e0000
> --- /dev/null
> +++ b/gas/testsuite/gas/mips/loongson-2f-2.s
> @@ -0,0 +1,10 @@
> +# Test workarounds selected by -mfix-loongson2f-nop
> +
> +	.text
> +	.set noreorder
> +
> +	nop
> +	b	1f
> +	nop
> +1:
> +	nop

Following on from above, let's test some implicit nops as well.
E.g. add ".align 5" to the end, so that you get 4 extra nops
inserted by gas.

> +/* Work around a possible cpu pipeline issue of Loongson2F */
> +const struct mips_opcode loongson2f_nop_insn =
> +{"nop.at",     "",         0x00200825, 0xffffffff, 0,              	INSN2_ALIAS,	I1    }; /* or at,at,zero */

I probably wasn't very clear, sorry, but in my review, I was actually
arguing _against_ using a name like "nop.at".  I was supporting your
original decision to use "nop" here.  (I was just suggesting nop.at
in case anyone else objected.  Noone's objected so far though.)

Minor detail, but let's make the comment:

/* Used to implement -mfix-loongson2f-nop.  */

which is more specific.

Richard



More information about the Binutils mailing list