[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

This is a partial fix for what seems like a central problem:
bitfields are expressed as (start length) but whether "start" is
highest or lowest bit-number depends on "lsb0?".  It looks like
that's an ungood design choice and causes several bugs, two
fixed below, I hope.  (The "bitrange-overlap?" thingy looks
harmless though).  If anyone would ask, I'd say convert all
bit-field numbering to be as for the lsb0? #f case: a bitfield
specified as (dnf f-op "" () 0 3) always starts at 0 and is 3
bits long everywhere, regardless of lsb0?.

I've only tested these fixes on the CRIS target (wip) which has
a base-insn size of 16 bits, but can have a 16-bit (and 8-bit
within 16-bits) or a 32-bit constant as operand, similar to
m68k.  I though it'd work to express those fields in the same
way as in m68k.cpu, e.g.:

(d68f f-simm8  "signed 8 bit immediate"  () 16 16 7  8  INT #f #f)
(d68f f-simm16 "signed 16 bit immediate" () 16 16 15 16 INT #f #f)
(d68f f-simm32 "signed 32 bit immediate" () 16 32 31 32 INT #f #f)

However, I doubt that a m68k CGEN sim would work without the
patch below.

There's still a related bug somewhere: with an operand based on
f-simm8 (dnop sconst8 "" () h-sint f-simm8), the insn is
calculated as if sconst8 is an 8-bit constant field, so pc is
updated by one byte for the field, not two.  It should update by
two bytes since I say that f-simm8 is a 16-bit field, right?
Same thing again would happen for m68k methinks.

Another bug: I *have* to use the CISC model above for those
beyond-base-insn fields, since CGEN thinks the 32-bit field is a
16-bit field at insn word-offset 32 with the trivial (dnf
f-simm32 "" () 47 32).  Guesses: CGEN mixes it up with the insn
bitsize (16 as stated) or maybe it's another "lsb0?" bit-order
ungoodness.  Probably the "FIXME: word-offset/word-length
computation needs work" in ifield.scm:-ifield-parse needs fixing
too.  I'm too tired to see straight.

Ok to commit?

2002-06-19  Hans-Peter Nilsson  <hp@axis.com>

	* types.scm (bitrange-overlap?): Handle lsb0?.
	* utils-gen.scm (-gen-extract-word): Ditto.

Index: types.scm
===================================================================
RCS file: /cvs/src/src/cgen/types.scm,v
retrieving revision 1.1.1.1
diff -p -c -r1.1.1.1 types.scm
*** types.scm	28 Jul 2000 04:11:52 -0000	1.1.1.1
--- types.scm	19 Jun 2002 00:10:19 -0000
***************
*** 241,251 ****
  ; Return a boolean indicating if two bitranges overlap.
  
  (define (bitrange-overlap? start1 length1 start2 length2 lsb0?)
!   ; ??? lsb0?
!   (let ((end1 (+ start1 length1))
! 	(end2 (+ start2 length2)))
!     (not (or (<= end1 start2)
! 	     (>= start1 end2))))
  )
  
  ; Return a boolean indicating if BITPOS is beyond bitrange START,LEN.
--- 241,255 ----
  ; Return a boolean indicating if two bitranges overlap.
  
  (define (bitrange-overlap? start1 length1 start2 length2 lsb0?)
!   (if lsb0?
!       (let ((end1 (- start1 length1))
! 	    (end2 (- start2 length2)))
! 	(not (or (<= start1 end2)
! 		 (>= end1 start2))))
!       (let ((end1 (+ start1 length1))
! 	    (end2 (+ start2 length2)))
! 	(not (or (<= end1 start2)
! 		 (>= start1 end2)))))
  )
  
  ; Return a boolean indicating if BITPOS is beyond bitrange START,LEN.
Index: utils-gen.scm
===================================================================
RCS file: /cvs/src/src/cgen/utils-gen.scm,v
retrieving revision 1.6
diff -p -c -r1.6 utils-gen.scm
*** utils-gen.scm	14 Nov 2001 19:46:43 -0000	1.6
--- utils-gen.scm	19 Jun 2002 00:10:20 -0000
***************
*** 116,125 ****
  
  (define (-gen-extract-word word-name word-start word-length start length
  			   unsigned? lsb0?)
!   ; ??? lsb0?
!   (let ((word-end (+ word-start word-length))
! 	(end (+ start length))
! 	(base (if (< start word-start) word-start start)))
      (string-append "("
  		   "EXTRACT_"
  		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
--- 116,131 ----
  
  (define (-gen-extract-word word-name word-start word-length start length
  			   unsigned? lsb0?)
!   ; Canonicalize on the low and high numbered ends of the field; use the
!   ; lsb?-adjusted numbering only when necessary.
!   (let* ((field-low (if lsb0? (- start length) start))
! 	 (field-high (if lsb0? start (+ start length)))
! 	 (word-low word-start)
! 	 (word-high (+ word-start word-length))
! 	 ; The field part within the extracted word.
! 	 (fieldpart-low (if (< field-low word-low) 0 (- field-low word-low)))
! 	 (fieldpart-high (if (> field-high word-high)
! 			     word-length (- field-high word-low))))
      (string-append "("
  		   "EXTRACT_"
  		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
***************
*** 133,148 ****
  		   ", "
  		   (number->string word-length)
  		   ", "
! 		   (number->string (if (< start word-start)
! 				       0
! 				       (- start word-start)))
  		   ", "
! 		   (number->string (if (< end word-end)
! 				       (- end base)
! 				       (- word-end base)))
  		   ") << "
! 		   (number->string (if (> end word-end)
! 				       (- end word-end)
  				       0))
  		   ")"))
  )
--- 139,150 ----
  		   ", "
  		   (number->string word-length)
  		   ", "
! 		   (number->string (if lsb0? fieldpart-high fieldpart-low))
  		   ", "
! 		   (number->string (+ 1 (- fieldpart-high fieldpart-low)))
  		   ") << "
! 		   (number->string (if (> field-high word-high)
! 				       (- field-high word-high)
  				       0))
  		   ")"))
  )

brgds, H-P

[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Doug Evans]

Hans-Peter Nilsson writes:
 > This is a partial fix for what seems like a central problem:
 > bitfields are expressed as (start length) but whether "start" is
 > highest or lowest bit-number depends on "lsb0?".  It looks like
 > that's an ungood design choice and causes several bugs, two
 > fixed below, I hope.  (The "bitrange-overlap?" thingy looks
 > harmless though).  If anyone would ask, I'd say convert all
 > bit-field numbering to be as for the lsb0? #f case: a bitfield
 > specified as (dnf f-op "" () 0 3) always starts at 0 and is 3
 > bits long everywhere, regardless of lsb0?.

One recognizes these issues going in.
One problem to consider is whether the .cpu writer
can enter the data using the numbering scheme found in the architecture
manual, or whether s/he has to translate to cgen's scheme.

Certainly .cpu derived machine generated documentation
should match the architecture manual, but that can be done
at document generation time.  The issue then becomes one
of maintenance.  Are people willing to put up with having
to continually do the translation for cpu's that happen
to pick the "wrong" (1/2 :-) numbering scheme.  And when they
make mistakes will they also argue there's a bug?

One alternative is to do the canonicalization at .cpu reading time,
but that can also lead to confusion (causing people to revisit
the issue again).

I'm not fixed on the current scheme, but it shouldn't change without
a full discussion and buy-in from people on the list.

It'll take a day to study the patch.

[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Doug Evans]

Hans-Peter Nilsson writes:
 > Ok to commit?
 > 
 > 2002-06-19  Hans-Peter Nilsson  <hp@axis.com>
 > 
 > 	* types.scm (bitrange-overlap?): Handle lsb0?.
 > 	* utils-gen.scm (-gen-extract-word): Ditto.
 > 
 > Index: types.scm
 > ===================================================================
 > RCS file: /cvs/src/src/cgen/types.scm,v
 > retrieving revision 1.1.1.1
 > diff -p -c -r1.1.1.1 types.scm
 > *** types.scm	28 Jul 2000 04:11:52 -0000	1.1.1.1
 > --- types.scm	19 Jun 2002 00:10:19 -0000
 > ***************
 > *** 241,251 ****
 >   ; Return a boolean indicating if two bitranges overlap.
 >   
 >   (define (bitrange-overlap? start1 length1 start2 length2 lsb0?)
 > !   ; ??? lsb0?
 > !   (let ((end1 (+ start1 length1))
 > ! 	(end2 (+ start2 length2)))
 > !     (not (or (<= end1 start2)
 > ! 	     (>= start1 end2))))
 >   )
 >   
 >   ; Return a boolean indicating if BITPOS is beyond bitrange START,LEN.
 > --- 241,255 ----
 >   ; Return a boolean indicating if two bitranges overlap.
 >   
 >   (define (bitrange-overlap? start1 length1 start2 length2 lsb0?)
 > !   (if lsb0?
 > !       (let ((end1 (- start1 length1))
 > ! 	    (end2 (- start2 length2)))
 > ! 	(not (or (<= start1 end2)
 > ! 		 (>= end1 start2))))
 > !       (let ((end1 (+ start1 length1))
 > ! 	    (end2 (+ start2 length2)))
 > ! 	(not (or (<= end1 start2)
 > ! 		 (>= start1 end2)))))
 >   )
 >   
 >   ; Return a boolean indicating if BITPOS is beyond bitrange START,LEN.


I don't know why bitrange-overlap? was written to use not/or instead
of just and.  Patch is approved, modulo converting it to this.

(define (bitrange-overlap? start1 length1 start2 length2 lsb0?)
  (if lsb0?
      (let ((end1 (- start1 length1))
	    (end2 (- start2 length2)))
	(and (< end1 start2)
	     (> start1 end2)))
      (let ((end1 (+ start1 length1))
	    (end2 (+ start2 length2)))
	(and (> end1 start2)
	     (< start1 end2))))
)

sound ok?

[utils-gen.scm patch discussion to follow]

[And sorry for the delay, I know I said a day.
Took awhile to update my fsf tree and get it building again.]

[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Doug Evans]

Not something of your making, but one thing that can be cleaned
up in -gen-extract-word is it's use of current-arch-insn-lsb0?.
Either it should use the lsb0? argument, or the lsb0? argument
should be removed.  I don't have a strong preference which.

[grep for >>>]

(define (-gen-extract-word word-name word-start word-length start length
			   unsigned? lsb0?)
  ; ??? lsb0?
  (let ((word-end (+ word-start word-length))
	(end (+ start length))
	(base (if (< start word-start) word-start start)))
    (string-append "("
		   "EXTRACT_"
>>>>>>>		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
		   (if (and (not unsigned?)
			    ; Only want sign extension for word with sign bit.
			    (bitrange-overlap? start 1 word-start word-length
					       lsb0?))
		       "_INT ("
		       "_UINT (")
		   word-name
		   ", "
		   (number->string word-length)
		   ", "
		   (number->string (if (< start word-start)
				       0
				       (- start word-start)))
		   ", "
		   (number->string (if (< end word-end)
				       (- end base)
				       (- word-end base)))
		   ") << "
		   (number->string (if (> end word-end)
				       (- end word-end)
				       0))
		   ")"))
)

[RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Doug Evans]

Hans-Peter Nilsson writes:
 > Ok to commit?
 > 
 > 2002-06-19  Hans-Peter Nilsson  <hp@axis.com>
 > 
 > 	* types.scm (bitrange-overlap?): Handle lsb0?.
 > 	* utils-gen.scm (-gen-extract-word): Ditto.
 > 
 > Index: utils-gen.scm
 > ===================================================================
 > RCS file: /cvs/src/src/cgen/utils-gen.scm,v
 > retrieving revision 1.6
 > diff -p -c -r1.6 utils-gen.scm
 > *** utils-gen.scm	14 Nov 2001 19:46:43 -0000	1.6
 > --- utils-gen.scm	19 Jun 2002 00:10:20 -0000
 > ***************
 > *** 116,125 ****
 >   
 >   (define (-gen-extract-word word-name word-start word-length start length
 >   			   unsigned? lsb0?)
 > !   ; ??? lsb0?
 > !   (let ((word-end (+ word-start word-length))
 > ! 	(end (+ start length))
 > ! 	(base (if (< start word-start) word-start start)))
 >       (string-append "("
 >   		   "EXTRACT_"
 >   		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
 > --- 116,131 ----
 >   
 >   (define (-gen-extract-word word-name word-start word-length start length
 >   			   unsigned? lsb0?)
 > !   ; Canonicalize on the low and high numbered ends of the field; use the
 > !   ; lsb?-adjusted numbering only when necessary.
 > !   (let* ((field-low (if lsb0? (- start length) start))
 > ! 	 (field-high (if lsb0? start (+ start length)))
 > ! 	 (word-low word-start)
 > ! 	 (word-high (+ word-start word-length))
 > ! 	 ; The field part within the extracted word.
 > ! 	 (fieldpart-low (if (< field-low word-low) 0 (- field-low word-low)))
 > ! 	 (fieldpart-high (if (> field-high word-high)
 > ! 			     word-length (- field-high word-low))))
 >       (string-append "("
 >   		   "EXTRACT_"
 >   		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
 > ***************
 > *** 133,148 ****
 >   		   ", "
 >   		   (number->string word-length)
 >   		   ", "
 > ! 		   (number->string (if (< start word-start)
 > ! 				       0
 > ! 				       (- start word-start)))
 >   		   ", "
 > ! 		   (number->string (if (< end word-end)
 > ! 				       (- end base)
 > ! 				       (- word-end base)))
 >   		   ") << "
 > ! 		   (number->string (if (> end word-end)
 > ! 				       (- end word-end)
 >   				       0))
 >   		   ")"))
 >   )
 > --- 139,150 ----
 >   		   ", "
 >   		   (number->string word-length)
 >   		   ", "
 > ! 		   (number->string (if lsb0? fieldpart-high fieldpart-low))
 >   		   ", "
 > ! 	>>>>>>>	   (number->string (+ 1 (- fieldpart-high fieldpart-low)))
 >   		   ") << "
 > ! 		   (number->string (if (> field-high word-high)
 > ! 				       (- field-high word-high)
 >   				       0))
 >   		   ")"))
 >   )

Are you sure you want the +1 here (grep for >>>)?
I didn't study the lsb0? = #t case, but for lsb0? = #f I compared the old/new
equations on paper and you don't want the +1.

I'm guessing the same would be true for lsb0? = #t.
[unless of course you rework the fieldpart high/low calculations]

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

> From: Doug Evans <dje@transmeta.com>
> Date: Fri, 21 Jun 2002 11:58:49 -0700 (PDT)

> I don't know why bitrange-overlap? was written to use not/or instead
> of just and.  Patch is approved, modulo converting it to this.

And I don't know why I didn't spot that.  Doh.

> sound ok?

Sure, will do.  Haven't tested it though.

brgds, H-P

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

> From: Doug Evans <dje@transmeta.com>
> Date: Fri, 21 Jun 2002 12:13:19 -0700 (PDT)

> Not something of your making, but one thing that can be cleaned
> up in -gen-extract-word is it's use of current-arch-insn-lsb0?.
> Either it should use the lsb0? argument, or the lsb0? argument
> should be removed.  I don't have a strong preference which.

I'll convert it to use the lsb0? argument.

brgds, H-P

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

> From: Doug Evans <dje@transmeta.com>
> Date: Fri, 21 Jun 2002 13:54:10 -0700 (PDT)

> Are you sure you want the +1 here (grep for >>>)?

Yes.  The highest bit-number minus the lowest bit-number is the
number of bits less one.  If the field starts at bit 2 and ends
at bit 2 it's one bit long.  Oh... it's everything else that is
wrong.  Oops.  :-(

> [unless of course you rework the fieldpart high/low calculations]

Yup.  Sorry, please ignore that patch.  New patch forthcoming.

brgds, H-P

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Doug Evans]

Hans-Peter Nilsson writes:
 > The highest bit-number minus the lowest bit-number is the
 > number of bits less one.

Assuming one defines the field to be [low,high].
Another legitimate definition, within a localized context such
as intermediates used to produce some final result, is [low,high).
I'm sure there are other contexts where such a definition is legitimate.

 > If the field starts at bit 2 and ends
 > at bit 2 it's one bit long.

Such a field can also be written, setting aside bit numbering issues, [2,3).

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

> From: Doug Evans <dje@transmeta.com>
> Date: Wed, 19 Jun 2002 11:05:18 -0700 (PDT)

> Hans-Peter Nilsson writes:
>  > This is a partial fix for what seems like a central problem:
>  > bitfields are expressed as (start length) but whether "start" is
>  > highest or lowest bit-number depends on "lsb0?".

> One alternative is to do the canonicalization at .cpu reading time,
> but that can also lead to confusion (causing people to revisit
> the issue again).

I'm mostly worried about bugs similar to those I found, with the
*internal* representation different for different "lsb?".
Though having the "external" (.cpu) representation different is
supposedly confusing more people than not (assuming that CGEN
attracts a larger audience than the current. ;-)

brgds, H-P

Re: [RFA:] Fix lsb? bug with insn fields beyond base insn size.

[Hans-Peter Nilsson]

> From: Doug Evans <dje@transmeta.com>
> Date: Mon, 24 Jun 2002 08:18:11 -0700 (PDT)

> Hans-Peter Nilsson writes:
>  > The highest bit-number minus the lowest bit-number is the
>  > number of bits less one.
> 
> Assuming one defines the field to be [low,high].
> Another legitimate definition, within a localized context such
> as intermediates used to produce some final result, is [low,high).
> I'm sure there are other contexts where such a definition is legitimate.

Ok, the comment above field-high and field-low needs expansion,
since "canonicalize on the low and high numbered ends of the
field; use the lsb?-adjusted numbering only when necessary" is
ambiguous, opening up to such off-by-one interpretation.  Never
mind, as mentioned I'll send a new patch.

brgds, H-P