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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-02-29 11:00:05 +0000]:

Hi Claudiu,

Thanks for your feedback.  I have some follow on questions:

> > 
> > +/* 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.

So do you see the 'ARC_CPU_DEFAULT' being defined to 'ARC700' for the
current stock arc binutils, and then set to 'NPS400' for the Mellanox
NPS400 binutils?

The name feels wrong to me, the default for both core arc, and nps400
would still be arc700, it's just in the nps400 case we have an
additional feature enabled; one that adds some additional
instructions.

If the default architecture for arc changes to arc700 as you suggest
below, then all uses of ARC_NPS400 disappear from gas, so the config
changes can also be dropped.

That leaves just a single use of ARC_NPS400 in the opcode library,
where we include the extension instructions.  One possibility would be
that I could create a define call ARC_OPCODE_EXTENSION_INSN_FILE or
some such.  This would be undefined for core arc, and defined to
"arc-nps400-tbl.h" for nps400, then in arc_opcodes[] we can say:

    #ifdef ARC_OPCODE_EXTENSION_INSN_FILE
    #include ARC_OPCODE_EXTENSION_INSN_FILE
    #endif

Would that be better?

If you still prefer ARC_CPU_DEFAULT, could you expand a little more on
how you see it being used please.

> 
> > +  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.

OK, I'll do that.

> 
> > +      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.

I'm not sure I like that idea.  Keeping different extensions separate
would be best I think.

The total number of extensions is likely to be pretty low, but the
number of entries in (this one at least) is likely to be ~100.
Keeping them separate will help provide more structure to the code,
making it more obvious which sets of extensions go together.

>                                        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.

Which instructions do they overlap with?  I thought that these new
instructions fitted into the gap in the arc architecture reserved for
after-market extensions (major opcode 0x09).

As an experiment I assembled the nps400 new instruction test case
using the nps400 binutils then disassemble using a clean, stock arc
binutils, all I see are ".long HEX-VALUE" lines, which is what I would
expect.

The desired behaviour at this stage is that a binutils configured for
nps400 would appear to be an arc700 binutils, just with the additional
instruction included.

>                                                       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.

If this ever became a problem then I think the correct solution would
be a new ELF header flag, in the same way we split ARCv2 in to ARCEM
and ARCHS, we'd split ARC700 into ARC700 and NPS400.  I'm hoping any
such flag changes can be put off until necessary.

> 
> > +/* 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?

I'll try to get get more code reuse into here where possible.  It
sounds like you're happy with the general approach, which is the main
thing :)

I'll post a revised patch set soon.

Thanks,
Andrew