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

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

[Hans-Peter Nilsson]

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
@@ -684,9 +684,9 @@ 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.

And this one:

@@ -364,8 +364,14 @@ 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 :
+            if ((base_insn & 0xf00) == 0x0)
+              { itype = CRISV10F_INSN_BCC_B; goto extract_sfmt_bcc_b; }
+            itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
+          case 14 :
+            if ((base_insn & 0xff00) == 0xe000)
+              { itype = CRISV10F_INSN_BA_B; goto extract_sfmt_ba_b; }
+            itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
           default : itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
           }
         }

Aren't those tests redundant when base_insn == entire_insn (so
to speak; there is no entire_insn as per the patch below)?
Should I have conditionalized the whole generated if-test?

The change below mimics the test in
sim-decode.scm:-gen-decode-fn where entire_insn is conditionally
declared as a parameter to @prefix@_decode.  Without it, I get
compilation errors for the undeclared entire_insn.
Ok to commit?

	* utils-sim.scm (-gen-decode-insn-entry): Correct last change for
	non-(adata-integral-insn? CURRENT-ARCH) case.

Index: utils-sim.scm
===================================================================
RCS file: /cvs/src/src/cgen/utils-sim.scm,v
retrieving revision 1.13
diff -p -u -r1.13 utils-sim.scm
--- utils-sim.scm	18 May 2005 21:52:57 -0000	1.13
+++ utils-sim.scm	2 Dec 2005 03:33:15 -0000
@@ -636,7 +636,9 @@
 			  ";\n")
 			 "")
 		     ; Generate code to check that all of the opcode bits for this insn match
-		     indent "    if ((entire_insn & 0x" (number->hex (insn-base-mask insn)) ") == 0x" (number->hex (insn-value insn)) ")\n" 
+		     indent "    if (("
+		     (if (adata-integral-insn? CURRENT-ARCH) "entire_insn" "base_insn")
+		     " & 0x" (number->hex (insn-base-mask insn)) ") == 0x" (number->hex (insn-value insn)) ")\n" 
 		     indent "      { itype = " (gen-cpu-insn-enum (current-cpu) insn) ";"
 		     (if (with-scache?)
 			 (if fn?

brgds, H-P

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

[Jim Blandy]

Hans-Peter Nilsson <hans-peter.nilsson@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
> @@ -684,9 +684,9 @@ 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
grateful to have found a relatively straightforward way to make it at
least produce the right field values.  If you are up to the task of
going into utils-gen.scm and making it produce minimal and correct
fetches, please do.

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

[Hans-Peter Nilsson]

> From: Jim Blandy <jimb@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?

[Dave Brolley]

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:
>
>@@ -364,8 +364,14 @@ 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 :
>+            if ((base_insn & 0xf00) == 0x0)
>+              { itype = CRISV10F_INSN_BCC_B; goto extract_sfmt_bcc_b; }
>+            itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
>+          case 14 :
>+            if ((base_insn & 0xff00) == 0xe000)
>+              { itype = CRISV10F_INSN_BA_B; goto extract_sfmt_ba_b; }
>+            itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
>           default : itype = CRISV10F_INSN_X_INVALID; goto extract_sfmt_empty;
>           }
>         }
>
>Aren't those tests redundant when base_insn == entire_insn (so
>to speak; there is no entire_insn as per the patch below)?
>Should I have conditionalized the whole generated if-test?
>  
>
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. 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.

>The change below mimics the test in
>sim-decode.scm:-gen-decode-fn where entire_insn is conditionally
>declared as a parameter to @prefix@_decode.  Without it, I get
>compilation errors for the undeclared entire_insn.
>Ok to commit?
>  
>
This change looks ok to me. Please commit it.

Thanks,
Dave

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

[Hans-Peter Nilsson]

> Date: Fri, 02 Dec 2005 14:13:26 -0500
> From: Dave Brolley <brolley@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.

Anyway, I briefly tested timing before/after a regened sim/cris,
and time difference seems to be in the noise, at least for the
test programs I used.  I guess most interesting codes fit nicely
in the scache or something.  So I'm only worried about
correctness with the widened opcode fetches and the opcode
checks as above.

> >The change below mimics the test in
> >sim-decode.scm:-gen-decode-fn where entire_insn is conditionally
> >declared as a parameter to @prefix@_decode.
> This change looks ok to me. Please commit it.

Done, thanks.

brgds, H-P

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

[Dave Brolley]

Hans-Peter Nilsson wrote:

>>Date: Fri, 02 Dec 2005 14:13:26 -0500
>>From: Dave Brolley <brolley@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.

Dave