Re: [PATCH 1/6] gas/arc: Modify structure used to hold opcodes

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

* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2016-04-04 08:21:11 +0000]:

> Hi Andrew,
> 
> > The assembler builds a hash to hold references to arc_opcode entries in
> > the arc_opcodes table.  This hash assumes that each mnemonic will always
> > appear in contiguous blocks within the arc_opcodes table, so all ADD
> > instruction will be together, all AND instructions will likewise be
> > together and so on.
> 
> Yes this is on purpose. Having the group, we ensure we choose the
> correct encoding. As you can see the grouping of a mnemonic is done
> accordingly to an order, such that the small immediate arguments are
> preferred before the long ones. Once, breaking that you need to do
> the sorting at runtime, which will increase the time a file is
> assembled.

I see what you mean, though I slightly disagree, it's the ordering,
not the grouping that is relied upon.  So long as any second group
only supports instructions that are not covered by an earlier group
then all will be fine.

Given that the arc assembler as currently written relies on the
programmer to get the ordering "just right" I don't (personally) see
restricting to one group as being that much of an advantage in getting
the ordering correct.  I guess you disagree, but

> 
> > 
> > The problem with this is that as different variations of arc are added,
> > then it is often more convenient to split instructions apart, so all the
> > base ADD instructions are together, but, variants of ADD specific to one
> > variation of arc are grouped with other instructions specific to that
> > arc variant.  The current data structures don't support this
> > configuration of instructions.
> 
> It may not be a good idea to overwrite the basic ARC ISA. Any ARC
> customer can choose to have a different name than the standard ISA
> names. Please can u reconsider this.

I don't really understand what you are suggesting here, but ...

>                                        I do not see any reason why a
> customer to use ADD mnemonic and not MYADD or something of a
> sort.

... I think you're saying, change the ISA to work around limitation in
the assembler :-/ That doesn't feel like a good answer, surely we
should just adapt the assembler until it is flexible enough to handle
whatever ISAs we need.

>       Allowing the overwriting/addnotation of the standard ISA
> mnemonics is opening a domain of pain.

On inspection you'll notice that the new instruction actually uses a
unique flag operand and so there will never be a chance of confusion
in this case.

Could you clarify who you think will suffer this "domain of pain"?
The tool-chain maintainer?  The customer of Synopsys who is adding the
new instructions?  The user of the final tools?  Or someone else?

In the example you gave above, if a customer can think of a new ADD
instruction that fills a gap not covered by the core ADD instruction
then I think they absolutely _should_ call it ADD, not MYADD.

Consider the end user of the tools, if this new ADD is just like
any other ADD, but happens to take different parameter types, then
having this one variant being called MYADD would be highly annoying,
and would feel like the instruction was glued onto the side of the
architecture, rather than being seamlessly integrated into it.  I
think that we can, and should, do better.

>                                         On top of everything, I miss
> tests for your proposed patch (1 to 4).

Sorry I was not explicit in the commit message, but there should be no
functional change from patches 1 through 4.  Patch 5 should have no
functional change given the current state of the assembler.  These
tests are therefore that everything that currently works should
continue to do so.

It is only in patch 6 where I add a new instruction (that relies on
patches 1 to 5) that I add a new test.

I think I see 3 possible ways forward:

  1. Keep the current patch series, understanding that the ordering
  restriction still applies even across disjoint groups of
  instructions, and accepting that as a know danger area in the
  assembler.  This would be my preferred solution.

  2. Move this one nps instruction (sync.{rd,wr}) into the main arc
  opcode table, placing it next the core 'sync' instruction.  I'd
  probably leave a comment in the nps decode table explaining this
  strange placement.

  3. Write a small program that reads in the arc decode tables and
  places the entries into a single unified order, which can then be
  read by the assembler and disassembler.

Interested to hear your thoughts,

thanks,
Andrew