RE: Broken SH2a patches

[Andrew STUBBS <andrew dot stubbs at st dot com>]

> > I agree that there may be some problem inserting it into 
> the diagram, but
> > the important ting is that it must be inserted into the 
> inheritance tree in
> > the code and that the |s (ors) are removed from the 
> instruction table (the
> > op32 stuff may be ok, but the others have to go).
> 
> Ok, here is a patch that I think fixes up the diagram and removes the 
> |'s from the opcodes table.  What do you think ?

It does address most of the problems I was talking about, but unfortunately
there are a number of problems with it.

1. Although you have adjusted the test results for the existing tests you
have not added new tests for the six new architectures. I think it would
reveal some worry results ...

2. What does this mean?

#define arch_sh2a_nofpu_fake1 (arch_sh2_base| arch_sh2a_base \
      |arch_sh_no_mmu|arch_sh_no_co)

How can it have two bases?

3. The diagram:

                  SH2
     .------------'|`--------------------.
    /              |                      \
!  SH2           SH2A                    SH2E
! (dsp)         (fake1)                    |
!  |              /\                       |
!  |             /  \                      |
!  |            /    \                     |
!  |      .----'     SH3                  SH2A
!  |     /          (nommu)              (fake2) 
!  |     |            /\                   /\
!  |     |           /  \                 /  \
!  |     |          /    \               /    \
!  |     |       SH2A     SH3           /      |
!  |     |      (fake3)   /|\          /       |
!  |     |       /\      / | \        /        |
!  |     |  ----'  \ .../  |  ----.  /         |
!  |     | /      . \      |       \/          |
!  |    SH2A     .  SH4    |       SH3E        |
!  |   (nofpu)  .  (nommu) |        |          |
!  |     |     .   (nofpu) |        |          |
!  |  ...+.....      |  .--'        |          |
!  | /   |   .       | /            |          |
!  SH3   |          SH4            SH2A        |
! (dsp)  |         (nofpu)        (fake4)      |
!  |     |           /\             /\         |
!  |     |          /  '---.    .--'. \  .-----'
!  |     |         /        \  /       \/
! -+---- .      SH4A         SH4       SH2A <------....
!  |           (nofpu)        |
!  |            /  \        .-'
!  |  .--------'    '---.  /
!  | /                   \/
! SH4AL                 SH4A
! (dsp)

The sh3_nommu introduces a number of instructions, including 'ldc
<REG_N>,SSR' etc., which are not present in the sh2a (according to the
manual from the Renesas website). Yet your diagram shows sh2a inheriting
these instructions. As far as I can tell fake3 should descend from fake1,
while sh4_nommu_nofpu descends from both fake3 and sh3nommu. Similarly fake4
looks like it should descend from fake2, not sh3e.

fake2 should also descend from fake1, in addition to sh2e, since it also has
the shifts.

You need to look at these inhertances carefully. I assume fake1 and fake2
genuinely can descend from sh2 and sh2e (but please check), but fake3 and
fake4 cannot descend from anything sh3.

It is a brave attempt at ASCII line routing! Too bad it had lost its former
clarity though :-(

4. This is wrong:

#define arch_sh2a_nofpu_sh3_nommu_up \
	(arch_sh2a_nofpu_fake1 | arch_sh2_up)
#define arch_sh2a_sh3e_up            \
	(arch_sh2a_fake2       | arch_sh3e_up)
#define arch_sh2a_nofpu_sh4_nommu_up \
	(arch_sh2a_nofpu_fake3 | arch_sh4_nommu_nofpu_up)
#define arch_sh2a_sh4_up             \
	(arch_sh2a_fake4       | arch_sh3e_up)
 
The first and fourth are upside down somehow and all of them are incomplete.

It should be (according to the diagram):

#define arch_sh2a_fake1_up \
	(arch_sh2a_nofpu_fake1 | arch_sh2a_nofpu_up | sh3_nommu_up)
#define arch_sh2a_fake2_up \
	(arch_sh2a_fake2       | arch_sh3e_up | arch_sh2a_up)
#define arch_sh2a_fake3_up \
	(arch_sh2a_nofpu_fake3 | arch_sh4_nommu_nofpu_up \
	| sh2a_nofpu_up)
#define arch_sh2a_fake4_up \
	(arch_sh2a_fake4       | arch_sh4_up | arch_sh2a_up)

That is, itself plus _all_ its immediate descendents. I have changed the
names to help me understand it. The way you had two names for the same thing
was confusing. The second names were perhaps preferable (if more wordy) over
'fake' but perhaps also less future proof?

Strictly speaking it may not be necessary to list arch_sh2a_up in
arch_fake2_up because it is implied by arch_sh3e_up, but it provides
documentation.

Of course, what I have put above is wrong because the diagram it is based
upon is wrong, but I did not want to confuse the matter any further.

Additionally you have not updated the entries for the other architectures.
E.g. arch_sh3e_up needs to include arch_sh2a_up, according to the diagram. 

I don't like the name 'fake', but I'm not sure what else to suggest. Perhaps
sh2+, sh2e+, sh2++ and sh2e++ (or sh2+shift, sh2e+shift+fsqrt,
sh2+shift+pref and sh2e+double). Of course '+' won't work in C ... hmmm.

Unfortunately a lot of libraries etc. will end up labelled with these 'fake'
architectures so it is important that the name means something.

> > My other concern is the arch_op32 thingy. I don't know what 
> it is for, or
> > what the affect on the architecture code is, but you should 
> be aware that it
> > will not be encoded into the elf flags and, therefore, will 
> not be available
> > at link time. If it is only intended to be for the 
> assembler then it may be
> > ok.
> 
> It is intended for the assembler and disassembler.  It has no 
> affect on 
> architecture selection.

Ah I see, this is something about the instruction encoding that has leaked
into the wrong field. Should this not be encoded into the 'nibbles' somehow?
If not, this is something that should be documented in the file. It does say
it is an 'abuse' but not why.

--
Andrew Stubbs
andrew.stubbs@st.com
andrew.stubbs@superh.com