RE: [PATCH] gas/opcodes: Add initial arc nps400 support

[Claudiu Zissulescu <Claudiu dot Zissulescu at synopsys dot com>]

H Andrew,

I have some small remarks regarding your patch. Please find them inserted.

> 
> +/* ARC NPS400 support. */
> +#undef ARC_NPS400
> +
My opinion is we do not need this define, or at least not in this form. If we want to support multiple selective cpu targets, maybe we should (un)define something like ARC_CPU_DEFAULT. Where this macro will be initialized with a desired cpu by the configure script file. This may make sense for example when the assembler needs to behave differently between two cpu configurations, but as far as I can see there is not the case for the time being. 

> +  if (!mach_type_specified_p)
> +    {
> +#ifdef ARC_NPS400
> +      arc_select_cpu ("arc700");
> +#else
> +      arc_select_cpu ("all");
> +#endif
> +    }
You can let the default cpu to arc700. The "all" stuff is just for testing purposes, and needed to be removed anyhow.

> +      arc)
> +        case ${target} in
> +          arc*-mellanox-*)
> +
> +$as_echo "#define ARC_NPS400 1" >>confdefs.h
> +
> +            ;;
> +        esac
> +        ;;
> +
See my above comments.

> +      arc)
> +        case ${target} in
> +          arc*-mellanox-*)
> +            AC_DEFINE(ARC_NPS400, 1, [ARC NPS400 support.])
> +            ;;
> +        esac
> +        ;;
> +
Likewise.

> +++ b/opcodes/arc-nps400-tbl.h
> @@ -0,0 +1,3 @@
> +/* movb<.f><.cl> */
> +{ "movb", 0x48010000, 0xf80f0000, ARC_OPCODE_ARC700 , ARITH, NONE, {
> +NPS_R_DST, NPS_R_SRC1, NPS_R_SRC2, NPS_BITOP_DST_POS,
> +NPS_BITOP_SRC_POS, NPS_BITOP_SIZE }, { C_NPS_F }}, { "movb",
> +0x48010000, 0xf80f0000, ARC_OPCODE_ARC700 , ARITH, NONE, {
> NPS_R_J_DST,
> +NPS_R_SRC2, NPS_BITOP_DST_POS, NPS_BITOP_SRC_POS,
> NPS_BITOP_SIZE }, {
> +C_NPS_F, C_NPS_CL }},

Probably the best will be to make a file common for all other users of ARC architecture which are having their own custom instructions. Hence, I would propose to rename arc-nps400-tbl.h to arc-ext-tbl.h or something of a sort. Also, if you would like those instructions to be enabled by a particular assembler option, you can use a new Subclass category (e.g., add NPS400 to insn_subclass_t) instead of NONE, and match against it. It may make sense as your opcodes are overlapping with other arc instructions. In this situation, the disassembler may choose the first ones instead of yours. However, in this case, we need to add an option to the disassembler as well.

> +/* ARC NPS400 Support: See comment near head of file.  */ static
> +unsigned insert_nps_dst (unsigned insn ATTRIBUTE_UNUSED,
> +               int value ATTRIBUTE_UNUSED,
> +               const char **errmsg ATTRIBUTE_UNUSED) {
> +  switch (value)
> +    {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +      insn |= value << 24;
> +      break;
> +    case 12:
> +    case 13:
> +    case 14:
> +    case 15:
> +      insn |= (value - 8) << 24;
> +      break;
> +    default:
> +      *errmsg = _("Register must be either r0-r3 or r12-r15.");
> +      break;
> +    }
> +  return insn;
> +}
> +
> +static int
> +extract_nps_dst (unsigned insn ATTRIBUTE_UNUSED,
> +                bfd_boolean * invalid ATTRIBUTE_UNUSED) {
> +  int value = (insn >> 24) & 0x07;
> +  if (value > 3)
> +    return (value + 8);
> +  else
> +    return value;
> +}
> +
> +static unsigned
> +insert_nps_j_dst (unsigned insn ATTRIBUTE_UNUSED,
> +                 int value ATTRIBUTE_UNUSED,
> +                 const char **errmsg ATTRIBUTE_UNUSED) {
> +  switch (value)
> +    {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +      insn |= value << 24;
> +      break;
> +    case 12:
> +    case 13:
> +    case 14:
> +    case 15:
> +      insn |= (value - 8) << 24;
> +      break;
> +    default:
> +      *errmsg = _("Register must be either r0-r3 or r12-r15.");
> +      break;
> +    }
> +  return insn;
> +}
This function looks similar with insert_nps_dst(). Is not possible to use a single function only?

> +static unsigned
> +insert_nps_dup_src1 (unsigned insn ATTRIBUTE_UNUSED,
> +                    int value ATTRIBUTE_UNUSED,
> +                    const char **errmsg ATTRIBUTE_UNUSED) {
> +  switch (value)
> +    {
> +    case 0:
> +    case 1:
> +    case 2:
> +    case 3:
> +      insn |= value << 24;
> +      break;
> +    case 12:
> +    case 13:
> +    case 14:
> +    case 15:
> +      insn |= (value - 8) << 24;
> +      break;
> +    default:
> +      *errmsg = _("Register must be either r0-r3 or r12-r15.");
> +      break;
> +    }
> +  return insn;
> +}
Also this looks identical with insert_nps_dst() function.

> 
>  const unsigned arc_num_operands = ARRAY_SIZE (arc_operands); @@ -
> 1198,6 +1423,10 @@ const unsigned arc_NToperand = FKT_NT;  const struct
> arc_opcode arc_opcodes[] =  {  #include "arc-tbl.h"
> +
> +#ifdef ARC_NPS400
> +#include "arc-nps400-tbl.h"
> +#endif
>  };
See my comments on ARC_NPS400.

Bottom line: I would drop the ARC_NPS400 construction at all, I would make the arc-ext-tbl.h file with my new instructions, keep the changes in tc-arc.c and optimize the ones in arc-opcodes.c. Eventually, I would add a new subclass for the extra instructions.

N.B. Although the current binutils arc backend is not supporting the .extInstruction and friends, we will try to add it soon.

Best,
Claudiu