Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

Dave Brolley's changes broke the CRIS sim in at least two
places: in CGEN (see further below) and by assuming that all
sims also have CGEN disassemblers (film at 11).  Tsk tsk.
*Please* regen, cgen-maint-rebuild and test all CGEN simulators
when hacking general changes into the CGEN sim machinery.  It's
not like there are too many...

Anyway, I think I unbroke it (clean test-results), but I'm not
sure this is the best change.  I don't like these diffs in the
generated sim (this one seemingly unrelated to the breakage,
maybe it's due to Jim Blandy's change on 2005-05-16):

Index: cpuv10.h
 <at>  <at>  -684,9 +684,9  <at>  <at>  struct scache {
   unsigned int length;
 #define EXTRACT_IFMT_MOVUCWR_CODE \
   length = 4; \
-  word_1 = GETIMEMUHI (current_cpu, pc + 2); \
+  word_1 = GETIMEMUSI (current_cpu, pc + 2); \
   f_operand2 = EXTRACT_LSB0_UINT (insn, 16, 15, 4); \
-  f_indir_pc__word = (0|(EXTRACT_LSB0_UINT (word_1, 16, 15, 16) << 0)); \
+  f_indir_pc__word = (0|(EXTRACT_LSB0_UINT (word_1, 32, 15, 16) << 0)); \
   f_mode = EXTRACT_LSB0_UINT (insn, 16, 11, 2); \
   f_opcode = EXTRACT_LSB0_UINT (insn, 16, 9, 4); \
   f_size = EXTRACT_LSB0_UINT (insn, 16, 5, 2); \

Why fetch more than needed?  Can't that cause spurious invalid
accesses at the end of defined memory?  Doesn't it slow down the
simulator?  (Hm, I guess I can measure that...)  I don't have
the full context of all cases when this can happen, but for the

Re: Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

Hans-Peter Nilsson <hans-peter.nilsson <at> axis.com> writes:
> Anyway, I think I unbroke it (clean test-results), but I'm not
> sure this is the best change.  I don't like these diffs in the
> generated sim (this one seemingly unrelated to the breakage,
> maybe it's due to Jim Blandy's change on 2005-05-16):
>
> Index: cpuv10.h
>  <at>  <at>  -684,9 +684,9  <at>  <at>  struct scache {
>    unsigned int length;
>  #define EXTRACT_IFMT_MOVUCWR_CODE \
>    length = 4; \
> -  word_1 = GETIMEMUHI (current_cpu, pc + 2); \
> +  word_1 = GETIMEMUSI (current_cpu, pc + 2); \
>    f_operand2 = EXTRACT_LSB0_UINT (insn, 16, 15, 4); \
> -  f_indir_pc__word = (0|(EXTRACT_LSB0_UINT (word_1, 16, 15, 16) << 0)); \
> +  f_indir_pc__word = (0|(EXTRACT_LSB0_UINT (word_1, 32, 15, 16) << 0)); \
>    f_mode = EXTRACT_LSB0_UINT (insn, 16, 11, 2); \
>    f_opcode = EXTRACT_LSB0_UINT (insn, 16, 9, 4); \
>    f_size = EXTRACT_LSB0_UINT (insn, 16, 5, 2); \
>
> Why fetch more than needed?  Can't that cause spurious invalid
> accesses at the end of defined memory?  Doesn't it slow down the
> simulator?  (Hm, I guess I can measure that...)  I don't have
> the full context of all cases when this can happen, but for the
> case above, it's for moving a 16-bit constant field
> zero-extended into a 32-bit register.

That could be due to my 2005-05-16 change.  I have to admit, I didn't
feel very confident in my understanding of that code at all, and I was

Re: Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

> From: Jim Blandy <jimb <at> redhat.com>
> Date: Thu, 01 Dec 2005 23:06:40 -0800

> That could be due to my 2005-05-16 change.  I have to admit, I didn't
> feel very confident in my understanding of that code at all, and I was
> grateful to have found a relatively straightforward way to make it at
> least produce the right field values.

Well, for all I know it did *before*, but as shown it seems
doubtful it does in all cases *after* your change.  As always,
it may be that your change only uncovered another bug of course.

>  If you are up to the task of
> going into utils-gen.scm and making it produce minimal and correct
> fetches, please do.

To have a fighting chance at this, I need to know what bug this
fixes.  When I read the patch it seemed more of a convenience-
change.  I wouldn't rule out this fixing what is actually a bug
in the .cpu file.  Is there a machine description and a small
program I can use?

I guess you somehow got into trouble with a big-endian sim, but
your patch refers to this problem in terms a bit too vaguely for
me to decipher.

brgds, H-P

Re: Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

Hans-Peter Nilsson wrote:

>Dave Brolley's changes broke the CRIS sim in at least two
>places: in CGEN (see further below) and by assuming that all
>sims also have CGEN disassemblers (film at 11).  Tsk tsk.
>*Please* regen, cgen-maint-rebuild and test all CGEN simulators
>when hacking general changes into the CGEN sim machinery.  It's
>not like there are too many...
>  
>
First of all, my apologies for breaking this. I did test a variety of 
SID and sim targets, but obviously not this one.

>Anyway, I think I unbroke it (clean test-results), but I'm not
>sure this is the best change.  I don't like these diffs in the
>generated sim (this one seemingly unrelated to the breakage,
>maybe it's due to Jim Blandy's change on 2005-05-16):
>  
>
Looks like Jim has responded to this.

>And this one:
>
> <at>  <at>  -364,8 +364,14  <at>  <at>  crisv10f_decode (SIM_CPU *current_cpu, I
>           case 11 : /* fall through */
>           case 12 : /* fall through */
>           case 13 : /* fall through */
>-          case 15 : itype = CRISV10F_INSN_BCC_B; goto extract_sfmt_bcc_b;
>-          case 14 : itype = CRISV10F_INSN_BA_B; goto extract_sfmt_ba_b;
>+          case 15 :

new cgen snapshot available

A new automated cgen CVS snapshot is available.
ftp://sources.redhat.com/pub/cgen/snapshots/cgen-20051203.tar.bz2
1254772 bytes

Re: Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

> Date: Fri, 02 Dec 2005 14:13:26 -0500
> From: Dave Brolley <brolley <at> redhat.com>

> The switch generator stops when it has tested enough bits to resolve 
> ambiguity among the defined insns, but since it wasn't going on to test 
> the remaining bits, the decoder was recognizing invalid insns as valid. 
> This additional test goes on to test all the fixed bits in each insn to 
> ensure that they are all correct.

I assume you mean with opcode definitions for

opcode=
0?0
1?0

(where opcode & 2 is don't care and opcode & 1 is invalid)
that there was *before* the extra generated code, only a test
generated for (opcode & 4) and that there's *now* (opcode & 5)
== 4 and (opcode & 5) == 0, with 1 and 5 flagged as invalid
insns; not that you mean that there are now checks for (opcode &
7) == 0 and (opcode & 7) == 4.

> It does appear that we could be 
> smarter about generating the test, however. In this case the tests 
> appear to be redundant. We could probably add some logic to test only 
> the untested bits and to not generate the additional test at all if all 
> of the bits have already been tested.

Recent GCC should optimize out any obviously redundant tests
with if-tests that are covered by an outer switch/case.

Re: Unbreak CRIS sim: fix -gen-decode-insn-entry. And why the widened mem fetch?

Hans-Peter Nilsson wrote:

>>Date: Fri, 02 Dec 2005 14:13:26 -0500
>>From: Dave Brolley <brolley <at> redhat.com>
>>    
>>
>>The switch generator stops when it has tested enough bits to resolve 
>>ambiguity among the defined insns, but since it wasn't going on to test 
>>the remaining bits, the decoder was recognizing invalid insns as valid. 
>>This additional test goes on to test all the fixed bits in each insn to 
>>ensure that they are all correct.
>>    
>>
>
>I assume you mean with opcode definitions for
>
>opcode=
>0?0
>1?0
>
>(where opcode & 2 is don't care and opcode & 1 is invalid)
>that there was *before* the extra generated code, only a test
>generated for (opcode & 4) and that there's *now* (opcode & 5)
>== 4 and (opcode & 5) == 0, with 1 and 5 flagged as invalid
>insns; not that you mean that there are now checks for (opcode &
>7) == 0 and (opcode & 7) == 4.
>  
>
Correct.

[cgen-ibld-dis] fill_cache vs variable sized opcodes

M32C opcodes range from one to ten bytes long, so occasionally
fill_cache would attempt to read a "word" that extended beyond the end
of the memory segment, and would fail (this shows up with "objdump
-d").

There was already a partial test for this, but it didn't account for
opcodes longer than a word.  This patch changes the logic to account
for both short opcodes and long-but-not-whole-words opcodes, although
it uses "min_insn_bitsize < base_insn_bitsize" as a generic test for
"variable length opcodes".

Ok?

	* cgen-ibld.in (extract_normal): Avoid memory range errors.

Index: cgen-ibld.in
===================================================================
RCS file: /cvs/src/src/opcodes/cgen-ibld.in,v
retrieving revision 1.18
diff -p -U3 -r1.18 cgen-ibld.in
--- cgen-ibld.in	1 Jul 2005 11:16:31 -0000	1.18
+++ cgen-ibld.in	7 Dec 2005 22:12:46 -0000
 <at>  <at>  -440,9 +440,8  <at>  <at>  extract_normal (CGEN_CPU_DESC cd,
      word_length may be too big.  */
   if (cd->min_insn_bitsize < cd->base_insn_bitsize)
     {
-      if (word_offset == 0
-	  && word_length > total_length)
-	word_length = total_length;

new cgen snapshot available

A new automated cgen CVS snapshot is available.
ftp://sources.redhat.com/pub/cgen/snapshots/cgen-20051210.tar.bz2
1258217 bytes

fix type mismatch

In renaming the ms1 port, I had a problem rebuilding the autogenerated source 
files.  string-append and string-length complained about being given a symbol. 
I'm using guile 1.6.7.  This patch fixes the three places it was necessary to 
stringize a 'mode'.

ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan <at> codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

2005-12-15  Nathan Sidwell  <nathan <at> codesourcery.com>

	* sid-cpu.scm (-gen-hw-stream-and-destream-fns): Stringize mode
	for concatenation.
	(-hw-gen-write-stack-decl): Likewise.

Index: sid-cpu.scm
===================================================================
RCS file: /cvs/src/src/cgen/sid-cpu.scm,v
retrieving revision 1.14
diff -c -3 -p -r1.14 sid-cpu.scm
*** sid-cpu.scm	28 Oct 2005 19:30:02 -0000	1.14
--- sid-cpu.scm	15 Dec 2005 16:22:09 -0000
*************** namespace  <at> arch <at>  {
*** 212,218 ****
  	 (write-stacks