[patch] Hacky fix for -ve shifts in m32r decode2.c for cgen 1.1

[patch] Hacky fix for -ve shifts in m32r decode2.c for cgen 1.1

[Doug Evans]

Hi.

I've been treating this bug as a blocker for the 1.1 release.

gdb's sim/m32r/decode2.c has things like:

          unsigned int val = (((insn >> -8) & (3 << 0)));

          unsigned int val = (((insn >> -13) & (3 << 0)));

cgen's decoder will pick bits beyond the current "working" word.
[One can look at this bug in various ways.  That description
"works for me".  YMMV.]

This is a hack to work around the issue.
It's a safe patch conditioned on actually seeing a negative shift.

Going forward, I think the thing to do is begin supporting treating insn masks
and values as lists, and use that for m32r.

Btw, has the sid m32r simulator been tested recently?
It's doing things like:

m32rbf.cxx:
	      else // pair of short instructions
		{
		  UHI first = insn >> 16;
		  sem->decode (this, pc, first, first);
		}

and

m32r-decode.cxx:
      unsigned int val = (((insn >> 24) & (15 << 4)) | ((insn >> 20) & (15 << 0)));
      switch (val)
      {
      case 0 :
        entire_insn = entire_insn >> 16;

Note that
- insn is being shifted by 16 tis and then by 24 bits,
- after entire_insn >>= 16, it's been shifted by 32 bits.

Am I missing something?

---

2009-07-09  Doug Evans  <dje@sebabeach.org>

	* utils-sim.scm (-gen-decode-bits): New argument `entire-val'.
	All callers updated.  Work around -ve shifts by referencing the
	entire value.

Index: utils-sim.scm
===================================================================
RCS file: /cvs/src/src/cgen/utils-sim.scm,v
retrieving revision 1.15.4.1
diff -u -p -r1.15.4.1 utils-sim.scm
--- utils-sim.scm	24 Jun 2009 14:57:46 -0000	1.15.4.1
+++ utils-sim.scm	9 Jul 2009 21:10:20 -0000
@@ -521,11 +521,14 @@
 ; LSB0? is non-#f if bit number 0 is the least significant bit.
 ; FIXME: START may not be handled right in words beyond first.
 ;
+; ENTIRE-VAL is passed as a hack for cgen 1.1 which would previously generate
+; negative shifts.  FIXME: Revisit for 1.2.
+;
 ; e.g. (-gen-decode-bits '(0 1 2 3 8 9 10 11) 0 16 "insn" #f)
 ; --> "(((insn >> 8) & 0xf0) | ((insn >> 4) & 0xf))"
 ; FIXME: The generated code has some inefficiencies in edge cases.  Later.
 
-(define (-gen-decode-bits bitnums start size val lsb0?)
+(define (-gen-decode-bits bitnums start size val entire-val lsb0?)
 
   ; Compute a list of lists of three numbers:
   ; (first bitnum in group, position in result (0=LSB), bits in result)
@@ -563,16 +566,23 @@
 			    (bits (caddr group))
 			    ; Difference between where value is and where
 			    ; it needs to be.
-			    ; FIXME: Need to handle left (-ve) shift.
 			    (shift (- (if lsb0?
 					  (- first bits -1)
 					  (- (+ start size) (+ first bits)))
 				      pos)))
-		     (string-append
-		      " | ((" val " >> " (number->string shift)
-		      ") & ("
-		      (number->string (- (integer-expt 2 bits) 1))
-		      " << " (number->string pos) "))")))
+		       ; FIXME: There should never be a -ve shift here,
+		       ; but it can occur on the m32r.  Compensate here
+		       ; with hack and fix in 1.2.
+		       (if (< shift 0)
+			   (begin
+			     (set! val entire-val)
+			     (set! shift (+ shift size))))
+		       ; END-FIXME
+		       (string-append
+			" | ((" val " >> " (number->string shift)
+			") & ("
+			(number->string (- (integer-expt 2 bits) 1))
+			" << " (number->string pos) "))")))
 		   groups))
      ")"))
 )
@@ -945,7 +955,8 @@
 ; else {}
 ; may well be less stressful on the compiler to optimize than small switch() stmts.
 
-(define (-gen-decoder-switch switch-num startbit decode-bitsize table-guts indent lsb0? invalid-insn fn?)
+(define (-gen-decoder-switch switch-num startbit decode-bitsize table-guts
+			     indent lsb0? invalid-insn fn?)
   ; For entries that are a single insn, we're done, otherwise recurse.
 
   (string-list
@@ -965,7 +976,8 @@
        (string-append indent "  unsigned int val = "))
    (-gen-decode-bits (dtable-guts-bitnums table-guts)
 		     (dtable-guts-startbit table-guts)
-		     (dtable-guts-bitsize table-guts) "insn" lsb0?)
+		     (dtable-guts-bitsize table-guts)
+		     "insn" "entire_insn" lsb0?)
    ";\n"
    indent "  switch (val)\n"
    indent "  {\n"

Re: [patch] Hacky fix for -ve shifts in m32r decode2.c for cgen 1.1

[Frank Ch. Eigler]

Doug Evans <dje@sebabeach.org> writes:

> [...]
> Btw, has the sid m32r simulator been tested recently?

Not as far as I know.

> It's doing things like:
> m32rbf.cxx:
> 	      else // pair of short instructions
> 		{
> 		  UHI first = insn >> 16;
> 		  sem->decode (this, pc, first, first);
> 		}
> and
> m32r-decode.cxx:
>       unsigned int val = (((insn >> 24) & (15 << 4)) | ((insn >> 20) & (15 << 0)));
>       switch (val)
>       {
>       case 0 :
>         entire_insn = entire_insn >> 16;
>
> Note that
> - insn is being shifted by 16 tis and then by 24 bits,

Dunny, maybe this particular switch does not activate in the first
place for 16-bit short instructions?

> - after entire_insn >>= 16, it's been shifted by 32 bits.

(But is the value used after this point?)

- FChE