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-03-01 20:53:25 +0000]:

> Hi Andrew,
> 
> > 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?
> 
> No, more like arc700 or archs. The nps400 is an arc700 with some
> extra instructions. What I am trying to do is to have a piece of
> code that is generic and mps400 is only one of the possible
> configurations. But this discussion may be for another time.

Given that you said in a previous mail to make the default for core
arc binutils be arc700, which suits nps400 fine, I think that this
change can be put off for another day.  I already have a test in place
that ensures that nps400 gets arc700 as the default, so if/when the
rest of the arc binutils wants a different default, we can make the
above change then.

>
> > 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?
>
> Looks nice.  You need to think how to test the nsp400 without
> special actions, if possible.

My intention is that testing nps400 would be done by configuring and
building the nps400 target.  All the arc/nps400 tests will be guarded
so they don't run on core arc binutils, so we'll see no additional
test failures (if you do this is a mistake on my part, please let me
know).

> 
> > > 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.
> 
> I see your point, I just want to avoid to have a separate file for
> each ARC custom configuration in the binutils (see my generality
> concern). Again, it is not a show stopper.

I think that if there's a significant amount of code, which there will
be in this case, and it's logically separate from other, similar code,
then placing it in a separate file is probably best.  I don't think we
should worry about too many files; if there really are that many arc
variants out there trying to get into binutils we can always move the
tables to support them into a sub-directory.  As you say, this should
probably wait until it becomes an issue.

> 
>  
> > >                                                       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.
> 
> Maybe the best is to have a section with all the relevant
> information in. However, as you show it, we are not in need of such
> extra addition to the toolchain.

OK, for now I'll not worry about this.  We can address it later if
needed.

Thanks for the review.  I'll roll a new version of the patch, and post
it soon.

Thanks,
Andrew