Re: [PATCH v1] Add Loongson2F specific NOP instruction

[Richard Sandiford <rdsandiford at googlemail dot com>]

Wu Zhangjin <wuzhangjin@gmail.com> writes:
> Thanks very much for your feedback, what about this one?

Looks good, thanks.  I agree that having a separate mips_opcode structure
for the special nop is the right way to go.  It does of course mean that
the instruction will be disassembled as "addu at,at,zero" rather than
"nop", but I think that's probably a good thing.  It makes it easier
to check that all traditional nops have been removed.

If others disagree, I suppose an alternative would be to add a new
opcode ("nop.at" or something), and make "nop" assemble as "nop.at"
when the -mfix-* option is specified.  But using nonstandard opcodes
might be confusing, so I personally prefer your approach.

I think there are two main issues:

- You can only use "addu at,at,zero" in 64-bit code if "at" holds
  a sign-extended 32-bit value.  The MIPS architecture says that
  the result is unpredictable otherwise.

  I realise that using "addu" like this is probably fine for Loongson,
  but the idea is that things like "-mips3 -mfix-loongson2f-nop"
  should work on all MIPS III-compatible targets.  "or at,at,zero"
  would be a better choice, assuming that it avoids the errata and
  doesn't adversely affect the pipeline.

- Although the patch should work for all nops that the assembler itself
  inserts, it won't affect "nop"s that appear in the assembly source.
  For that, I think you'd need to make "nop" a macro.  I.e. you'd need
  to insert a "nop" INSN_MACRO entry in the mips-opc.c table (before the
  "real" nop entry) and handle the new macro in tc-mips.c:macro().
  You'd need to make md_begin skip this macro entry when setting up
  nop_insn.

You should document the new -mfix-* option in gas/c-mips.texi.

A gas/testsuite/gas/mips/ testcase would be useful to verify that both
implicit and explicit nops are handled correctly.  This helps people
who work on the code in future to tell whether the option still works
after their change.

A couple of more minor things:

- Other -mfix-* options are named after march=* options; that is,
  -mfix-foo is associated with -march=foo.  Since the -march option
  for Loongson 2F is -march=loongson2f rather than -march=ls2f, I think
  -mfix-loongson2f-nop would be a better name than -mfix-ls2f-nop.

  The names of the variable should be adjusted accordingly.
  How about mips_fix_loongson2f_nop for tc-mips.c and
  loongson2f_nop_insn for the new mips_opcode structure?

- Regarding this hunk:

> @@ -1917,7 +1920,10 @@ md_begin (void)
>  		broken = 1;
>  	      if (nop_insn.insn_mo == NULL && strcmp (name, "nop") == 0)
>  		{
> -		  create_insn (&nop_insn, mips_opcodes + i);
> +		  if (mips_fix_ls2f_nop)
> +			  create_insn (&nop_insn, &ls2f_builtin_nop);
> +		  else
> +			  create_insn (&nop_insn, mips_opcodes + i);
>  		  nop_insn.fixed_p = 1;

  the GNU coding standards say that the create_insn calls should only
  be indented by two spaces relative to the "if".  See other code in
  tc-mips.c for a guide.

- In the changelog, all entries should be relative to the ChangeLog file.
  Entries for config/tc-mips.c go in gas/ChangeLog, so the entry should
  be "* config/tc-mips.c" rather than "* gas/config/tc-mips.c".
  Also, the log should mention every variable or function that has
  been changed; see the existing entries for examples.

  The changelog for the existing patch might have started like:

	* config/tc-mips.c (mips_fix_ls2f_nop): New variable.
	(md_begin): Initialize nop_insn from ls2f_builtin_nop
	when -mfix-ls2f-nop is passed.

However, this issue still needs to be sorted out first:

>> Any patch along those lines will need a copyright assignment.  Do you
>> (or your employer) have one on file?

That is, copyright to all GCC and binutils changes must be assigned
to the FSF before they can be accepted.  Do you or your employer have
an FSF copyright assignment on file?  (I don't have access to the list,
sorry, but Nick (the binutils maintainer) does.)

Richard