Re: [PATCH] MIPS: microMIPS and MCU ASE instruction set support

[Richard Sandiford <rdsandiford@googlemail.com>]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
>  I'll look into whatever's left yet.  Here's the current version for a 
> reference, in case you or anyone else wanted to comment on what I have 
> modified so far (no obligation, of course!).

OK, finally got chance to have a proper read of the changes from
the previous patch, and they look good, thanks.  I'm afraid most
of this is at the level of very minor niggly stuff:

+/* Whether code compression (either of the MIPS16 or the microMIPS ASEs has

s/ASEs has/ASEs) has/

+  if (mips_opts.micromips)
+    return is_micromips_16bit_p (insn->insn_mo)
+         ? 2 : (is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);

This formatting isn't quite right because of the "emacs brackets" rule.
Should be:

    return (is_micromips_16bit_p (insn->insn_mo)
            ? 2 : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);

or:

    return (is_micromips_16bit_p (insn->insn_mo) ? 2
            : is_micromips_32bit_p (insn->insn_mo) ? 4 : 6);

But I'd be happy with an if-return-if-return-return chain too.
See below about these insns though...

-+  if (!mips_opts.mips16 && !mips_opts.micromips)
++  if (! HAVE_CODE_COMPRESSION)

The GCC decision a few years back was that no space should be added
after a unary operator (as with pointer deference, etc).  Not important
enough to do a sed on the whole source base, but we might as well avoid
changes that go the other way (from no space to space) in GAS.

+static bfd_boolean
+is_size_valid (const struct mips_opcode *mo)
+{
+  gas_assert (mips_opts.micromips);
+
+  if ((micromips_16 || micromips_32) && mo->pinfo == INSN_MACRO)
+    return FALSE;
+  if (micromips_16 && ! is_micromips_16bit_p (mo))
+    return FALSE;
+  if (micromips_32 && ! is_micromips_32bit_p (mo))
+    return FALSE;
+
+  return TRUE;
+}

Hmm, seeing this highlighted more makes me wonder whether
micromips_16 and micromips_32 shouldn't be combined into a
single variable that represents "the size the user set",
with 0 meaning "none".  As it stands, we'll have checks like:

  micromips_16 || micromips_32 || micromips_48

when any future 48-bit support is added.  Having separate variables
also gives the impression that arbitrary combinations are possible.

Also, how about replacing is_micromips_XXbit_p with a function
that just returns the size of a micromips insn?  We generally
deal with byte rather than bit lengths, so both this new function
and the combined "the size the user set" variable should probably
both be byte values.

Seems the code would be a fair bit clearer with those changes,
but maybe I'm wrong...

Maybe the mo->match assertions in is_micromips_XXbit_p are better
done in validate_micromips_insn -- especially if that makes the
changes above easier -- but I don't mind either way.

char
mips_nop_opcode (void)
{
  if (seg_info (now_seg)->tc_segment_info_data.micromips)
    return NOP_OPCODE_MICROMIPS;
 
  return seg_info (now_seg)->tc_segment_info_data.mips16
        ? NOP_OPCODE_MIPS16 : NOP_OPCODE_MIPS;
}

Same "emacs brackets" thing here.  Seems odd to treat microMIPS and MIPS16
differently like this, so I think if-return-if-return-return makes more
sense.

The new mips_handle_align looks very good, thanks.  I'm afraid it's another
silly nitpick, but usual style is not to have the brackets in "*(p++)".

Richard