[PATCH/committed] sim: mips: fix builds for r3900 cpus due to missing check_u64

  ---
 sim/mips/ChangeLog | 4 ++++
 sim/mips/mips.igen | 1 +
 2 files changed, 5 insertions(+)
  

On Fri, 11 Nov 2016, Mike Frysinger wrote:

> diff --git a/sim/mips/mips.igen b/sim/mips/mips.igen
> index 53370bf3dcf6..522cad6fe450 100644
> --- a/sim/mips/mips.igen
> +++ b/sim/mips/mips.igen
> @@ -496,6 +496,7 @@
>  *vr5000:
>  *vr5400:
>  *vr5500:
> +*r3900:
>  {
>    // The check should be similar to mips64 for any with PX/UX bit equivalents.
>  }

 This looks wrong to me -- as far as the instruction set is concerned the 
TX39 family members are all 32-bit MIPS I processors with some extensions 
(branch likelies, MADD/U, CACHE, SYNC, DERET and SDBBP, except the latter 
all compatibly encoded) and load interlocking (no load delay slots) [1] -- 
so mostly like `mipsI' -- and therefore `check_u64' shouldn't be called in 
the first place.

 If the simulator fails to build, then the bug must be elsewhere, e.g. why 
are DMFC1 and DMTC1 marked for `r3900' given that there are no 64-bit GPRs 
that could be used by these instructions -- a misapplied patch hunk in a 
distant past perhaps?

 It looks to me like this change needs be reverted and the `r3900' marking 
removed from DMFC1/DMTC1 encodings and their dependencies instead.

References:

[1] "32-Bit TX System RISC TX39 Family Architecture", Toshiba Corporation,
    Appendix A "Instruction Set Details"

 HTH,

  Maciej
  

On 17 Nov 2016 23:13, Maciej W. Rozycki wrote:
> On Fri, 11 Nov 2016, Mike Frysinger wrote:
> > --- a/sim/mips/mips.igen
> > +++ b/sim/mips/mips.igen
> > @@ -496,6 +496,7 @@
> >  *vr5000:
> >  *vr5400:
> >  *vr5500:
> > +*r3900:
> >  {
> >    // The check should be similar to mips64 for any with PX/UX bit equivalents.
> >  }
> 
>  This looks wrong to me -- as far as the instruction set is concerned the 
> TX39 family members are all 32-bit MIPS I processors with some extensions 
> (branch likelies, MADD/U, CACHE, SYNC, DERET and SDBBP, except the latter 
> all compatibly encoded) and load interlocking (no load delay slots) [1] -- 
> so mostly like `mipsI' -- and therefore `check_u64' shouldn't be called in 
> the first place.
> 
>  If the simulator fails to build, then the bug must be elsewhere, e.g. why 
> are DMFC1 and DMTC1 marked for `r3900' given that there are no 64-bit GPRs 
> that could be used by these instructions -- a misapplied patch hunk in a 
> distant past perhaps?
> 
>  It looks to me like this change needs be reverted and the `r3900' marking 
> removed from DMFC1/DMTC1 encodings and their dependencies instead.

the mips ISAs are too varied for me to try and dive in and find the right
answer, and you're certainly way more familiar than i here.  so whatever
you want to do here is fine :).
-mike
  

On Fri, 18 Nov 2016, Mike Frysinger wrote:

> >  It looks to me like this change needs be reverted and the `r3900' marking 
> > removed from DMFC1/DMTC1 encodings and their dependencies instead.
> 
> the mips ISAs are too varied for me to try and dive in and find the right
> answer, and you're certainly way more familiar than i here.  so whatever
> you want to do here is fine :).

 You're always welcome to ask and I'll be happy to assist you with any 
MIPS issues.  And if you cc me on mailing list postings, then it'll make 
it easier to me to give a timely response and certainly not to miss any 
altogether, as I'm not always up to date with tracking general traffic.

 I've had a closer look now and uncovered a whole bunch of ISA subsetting 
and wrong conditioning issues.  I've ended up with some half a dozen 
patches, all of which should be fairly obvious if accompanied with decent 
descriptions I yet need to write down, although most touch a fair amount 
of igen markup and actual code.

 Which means I'd be more confident about pushing them if I had a way to 
verify them -- do you have a testing procedure established for verifying 
GNU sim patches, MIPS or otherwise, that I could reuse?

  Maciej
  

On Wed, 23 Nov 2016, Maciej W. Rozycki wrote:

>  Which means I'd be more confident about pushing them if I had a way to 
> verify them -- do you have a testing procedure established for verifying 
> GNU sim patches, MIPS or otherwise, that I could reuse?

 Ping!

  Maciej
  

On 23 Nov 2016 14:41, Maciej W. Rozycki wrote:
> On Fri, 18 Nov 2016, Mike Frysinger wrote:
> > >  It looks to me like this change needs be reverted and the `r3900' marking 
> > > removed from DMFC1/DMTC1 encodings and their dependencies instead.
> > 
> > the mips ISAs are too varied for me to try and dive in and find the right
> > answer, and you're certainly way more familiar than i here.  so whatever
> > you want to do here is fine :).
> 
>  You're always welcome to ask and I'll be happy to assist you with any 
> MIPS issues.  And if you cc me on mailing list postings, then it'll make 
> it easier to me to give a timely response and certainly not to miss any 
> altogether, as I'm not always up to date with tracking general traffic.

sorry, i've gotten used to mips being orphaned.  i'll try to fix that
muscle memory and loop you in sooner.

would you mind adding yourself to the sim/MAINTAINERS list ?

>  I've had a closer look now and uncovered a whole bunch of ISA subsetting 
> and wrong conditioning issues.  I've ended up with some half a dozen 
> patches, all of which should be fairly obvious if accompanied with decent 
> descriptions I yet need to write down, although most touch a fair amount 
> of igen markup and actual code.

for these igen files, certainly feel free to jump in and push stuff.

>  Which means I'd be more confident about pushing them if I had a way to 
> verify them -- do you have a testing procedure established for verifying 
> GNU sim patches, MIPS or otherwise, that I could reuse?

sorry, i had missed this last part.  i was just waiting for you to post
patches for me to rubber stamp :).

a big problem with the mips sim is that it tailors its build heavily
based on the target.  most other sims enable everything and then wait
for cpu selection at runtime.  this is why some mips builds work fine
but others break and it's a while before anyone notices+reports.

if you look at sim/mips/configure.ac you'll see a large number of
checks on $target.  basically every matrix in there needs to be built.
i do:
	mkdir build
	cd build
	mkdir <mips-tuple>
	cd <mips-tuple>
	../../configure \
		--disable-gdbtk --disable-readline --with-system-readline \
		--with-system-zlib --disable-nls \
		--target=<mips-tuple>
	make all-sim -j4
	make check-sim
	make all-gdb  # If you really want, but prob not needed.

i would look at a check-sim as a good signal that things are OK, but
not a great one.  the number of tests in sim/testsuite/sim/mips/ is
too low (although mips32-dsp2 is a nice example of large coverage).

the three tuples i use currently are:
	mips-elf
	mips-sde-elf
	mipstx39-rtems4.12
but as you can see from that configure flag, i'm missing coverage.
but i don't care that much as i rarely (if ever) touch igen files.
my focus is on the common code/drivers.
-mike
  

On Thu, 8 Dec 2016, Mike Frysinger wrote:

> >  You're always welcome to ask and I'll be happy to assist you with any 
> > MIPS issues.  And if you cc me on mailing list postings, then it'll make 
> > it easier to me to give a timely response and certainly not to miss any 
> > altogether, as I'm not always up to date with tracking general traffic.
> 
> sorry, i've gotten used to mips being orphaned.  i'll try to fix that
> muscle memory and loop you in sooner.

 Hmm, I have offered you assistance with the MIPS port previously, earlier 
this year, although perhaps I wasn't clear enough.

> would you mind adding yourself to the sim/MAINTAINERS list ?

 Done now, after some consideration, and pushed with the most recent batch 
of changes for upcoming binutils 2.28.

 The thing is I'm already quite loaded with my commitment to binutils and 
GDB MIPS port maintenance, and then other activities related to the MIPS 
target.  We need to get the MIPS port back on track though and it seems 
like there is no other person available to drive that effort right now.

 So I take this post to address this problem, with the intent to 
ultimately help growing someone else as the primary MIPS port maintainer, 
perhaps also from Imagination, by reviewing their patches until we are 
confident that person is suitable.  I'll do the best I can in these 
circumstances and also please do not hesitate to ask me if you have any 
questions or requirements for the MIPS port.

> >  I've had a closer look now and uncovered a whole bunch of ISA subsetting 
> > and wrong conditioning issues.  I've ended up with some half a dozen 
> > patches, all of which should be fairly obvious if accompanied with decent 
> > descriptions I yet need to write down, although most touch a fair amount 
> > of igen markup and actual code.
> 
> for these igen files, certainly feel free to jump in and push stuff.

 Sure; except from very basic obvious changes I'd prefer to have a way to 
validate updates though.

> >  Which means I'd be more confident about pushing them if I had a way to 
> > verify them -- do you have a testing procedure established for verifying 
> > GNU sim patches, MIPS or otherwise, that I could reuse?
> 
> sorry, i had missed this last part.  i was just waiting for you to post
> patches for me to rubber stamp :).
> 
> a big problem with the mips sim is that it tailors its build heavily
> based on the target.  most other sims enable everything and then wait
> for cpu selection at runtime.  this is why some mips builds work fine
> but others break and it's a while before anyone notices+reports.

 Indeed, I've noticed that peculiarity.  The motivation was I suppose the 
desire to avoid including support for irrelevant hardware, which might 
interfere or affect performance.

> if you look at sim/mips/configure.ac you'll see a large number of
> checks on $target.  basically every matrix in there needs to be built.
> i do:
> 	mkdir build
> 	cd build
> 	mkdir <mips-tuple>
> 	cd <mips-tuple>
> 	../../configure \
> 		--disable-gdbtk --disable-readline --with-system-readline \
> 		--with-system-zlib --disable-nls \
> 		--target=<mips-tuple>
> 	make all-sim -j4
> 	make check-sim
> 	make all-gdb  # If you really want, but prob not needed.
> 
> i would look at a check-sim as a good signal that things are OK, but
> not a great one.  the number of tests in sim/testsuite/sim/mips/ is
> too low (although mips32-dsp2 is a nice example of large coverage).

 Ack.  I think verifying actual software is a way to improve test suite 
coverage -- any issue discovered there can be converted to a test suite 
case and included with GNU sim so that we don't have to rely on external 
testing.  I know GNU sim can be used to run GDB testing for example, which 
is a way to watch out for regressions.  It's been a long while though 
since I last did that.

> the three tuples i use currently are:
> 	mips-elf
> 	mips-sde-elf
> 	mipstx39-rtems4.12
> but as you can see from that configure flag, i'm missing coverage.
> but i don't care that much as i rarely (if ever) touch igen files.
> my focus is on the common code/drivers.

 OK, thanks for the directions.  I'll look into the details and see if I 
can get set up once I'm done with the current binutils 2.28 rush; sometime 
after the New Year.  I'll post the outstanding patches then too.

  Maciej