[patch][rfa] -opcode-slots: Handling of short insns

[patch][rfa] -opcode-slots: Handling of short insns

[Dave Brolley]

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

Hi,

This patch fixes a problem I ran across while working on an internal 
port. Incorrect opcodes and masks were sometimes being generated for the 
short insns of variable length ISAs because of two problems in 
-opcode-slots:

1) The test of bit positions against the insn-length was off by one. 
Thjs could lead to incorrect opcode bits being generated.

2) While the 'compute' function correctly generated zeroes for the extra 
bits when computing 'opcode', it was also generating zeroes for these 
bits when computing 'opcode-mask', thus rendering these bits irrelevent. 
This could lead to the generation of numerous unreachable cases in the 
generated decoder switch.

The patch corrects the comparison of bit position against the 
insn-length and also allows the caller of 'compute' to specify the 
default bit value which should be generated bits beyond the length of a 
short insn. This allows 0 to be specified when computing 'opcode' and 1 
to be specified when computing opcode-mask. The patch also changes some 
of the logit calls to print values in hex which is more appropriate when 
examining bitmasks

This patch corrects the problem encountered with my internal port. I 
know of no other port which is affected by this bug. I have tested it 
against frv and xstormy16 and verified no changes to the generated decoders.

OK to commit?

Dave

[-- Attachment #2: decode.ChangeLog --]
[-- Type: text/plain, Size: 221 bytes --]

2004-01-28  Dave Brolley  <brolley@redhat.com>

	* decode.scm (-opcode-slots): For short insns, generate 'opcode' with
	zeroes in the extra bit positions and generate 'opcode-mask' with ones
	in the extra bit positions.


[-- Attachment #3: src040128.ChangeLog --]
[-- Type: text/plain, Size: 1711 bytes --]

cgen/ChangeLog:
2004-01-26  Dave Brolley  <brolley@redhat.com>

	* decode.scm (-opcode-slots): For short insns, generate 'opcode' with
	zeroes in the extra bit positions and generate 'opcode-mask' with ones
	in the extra bit positions.

cgen/cpu/ChangeLog.RedHat:
2004-01-26  Dave Brolley  <brolley@redhat.com>

	* mep.opc (OPTION_MASK): Remove dangerous whitspace following a
	backslash intended as a line continuation.

sid/main/dynamic/ChangeLog.RedHat:
2004-01-26  Dave Brolley  <brolley@redhat.com>

	* mepCfg.h (allocate_timer, configure_timer): New methods of MepBoardCfg.
	(set_opt_timer_channel_bitw): New method of MepBoardCfg.
	(timer_channel_bitw): New member of MepBoardCfg.
	* mepCfg.cxx (MepBoardCfg): Initialize timer_channel_bitw.
	(set_dmem_bank_num): Allow dmem_bank_num to be set to zero.
	(map_imem_dmem): Correct fmem_base_address for case of no dmem.
	Initialize dmem_base[0].
	(MepBoardCfg::write_config): Call configure_timer. Move setup of timer
	interrupt pins to configure_timer. Handle case where dmem_bank_num is
	zero.
	(add_timer): Move allocation, scheduling and connection of timer to
	allocate_timer and configure_timer.
	(allocate_timer, configure_timer): New methods of MepBoardCfg.
	(configure_dmac): Use timer_channel_bitw.
	(set_opt_timer_channel_bitw): New method of MepBoardCfg.
	* mainDynamic.cxx (usage): Document --timer-channel-bitw.
	(option_num): Add opt_timer_channel_bitw.
	(long_options): Add timer-channel-bitw.
	(main): Handle opt_timer_channel_bitw.
	* commonCfg.h (set_opt_timer_channel_bitw): New prototype.

utils/mep/ChangeLog:
2004-01-26  Dave Brolley  <brolley@redhat.com>

	* mepcfgtool.c (generate_simulator_script): Generate
	--timer-channel-bitw

Re: [patch][rfa] -opcode-slots: Handling of short insns

[Dave Brolley]

[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]

Attached the wrong file.....

Dave Brolley wrote:

> Hi,
>
> This patch fixes a problem I ran across while working on an internal 
> port. Incorrect opcodes and masks were sometimes being generated for 
> the short insns of variable length ISAs because of two problems in 
> -opcode-slots:
>
> 1) The test of bit positions against the insn-length was off by one. 
> Thjs could lead to incorrect opcode bits being generated.
>
> 2) While the 'compute' function correctly generated zeroes for the 
> extra bits when computing 'opcode', it was also generating zeroes for 
> these bits when computing 'opcode-mask', thus rendering these bits 
> irrelevent. This could lead to the generation of numerous unreachable 
> cases in the generated decoder switch.
>
> The patch corrects the comparison of bit position against the 
> insn-length and also allows the caller of 'compute' to specify the 
> default bit value which should be generated bits beyond the length of 
> a short insn. This allows 0 to be specified when computing 'opcode' 
> and 1 to be specified when computing opcode-mask. The patch also 
> changes some of the logit calls to print values in hex which is more 
> appropriate when examining bitmasks
>
> This patch corrects the problem encountered with my internal port. I 
> know of no other port which is affected by this bug. I have tested it 
> against frv and xstormy16 and verified no changes to the generated 
> decoders.
>
> OK to commit?
>
> Dave
>
>------------------------------------------------------------------------
>
>2004-01-28  Dave Brolley  <brolley@redhat.com>
>
>	* decode.scm (-opcode-slots): For short insns, generate 'opcode' with
>	zeroes in the extra bit positions and generate 'opcode-mask' with ones
>	in the extra bit positions.
>
>  
>
>------------------------------------------------------------------------
>
>cgen/ChangeLog:
>2004-01-26  Dave Brolley  <brolley@redhat.com>
>
>	* decode.scm (-opcode-slots): For short insns, generate 'opcode' with
>	zeroes in the extra bit positions and generate 'opcode-mask' with ones
>	in the extra bit positions.
>
>cgen/cpu/ChangeLog.RedHat:
>2004-01-26  Dave Brolley  <brolley@redhat.com>
>
>	* mep.opc (OPTION_MASK): Remove dangerous whitspace following a
>	backslash intended as a line continuation.
>
>sid/main/dynamic/ChangeLog.RedHat:
>2004-01-26  Dave Brolley  <brolley@redhat.com>
>
>	* mepCfg.h (allocate_timer, configure_timer): New methods of MepBoardCfg.
>	(set_opt_timer_channel_bitw): New method of MepBoardCfg.
>	(timer_channel_bitw): New member of MepBoardCfg.
>	* mepCfg.cxx (MepBoardCfg): Initialize timer_channel_bitw.
>	(set_dmem_bank_num): Allow dmem_bank_num to be set to zero.
>	(map_imem_dmem): Correct fmem_base_address for case of no dmem.
>	Initialize dmem_base[0].
>	(MepBoardCfg::write_config): Call configure_timer. Move setup of timer
>	interrupt pins to configure_timer. Handle case where dmem_bank_num is
>	zero.
>	(add_timer): Move allocation, scheduling and connection of timer to
>	allocate_timer and configure_timer.
>	(allocate_timer, configure_timer): New methods of MepBoardCfg.
>	(configure_dmac): Use timer_channel_bitw.
>	(set_opt_timer_channel_bitw): New method of MepBoardCfg.
>	* mainDynamic.cxx (usage): Document --timer-channel-bitw.
>	(option_num): Add opt_timer_channel_bitw.
>	(long_options): Add timer-channel-bitw.
>	(main): Handle opt_timer_channel_bitw.
>	* commonCfg.h (set_opt_timer_channel_bitw): New prototype.
>
>utils/mep/ChangeLog:
>2004-01-26  Dave Brolley  <brolley@redhat.com>
>
>	* mepcfgtool.c (generate_simulator_script): Generate
>	--timer-channel-bitw
>
>  
>


[-- Attachment #2: decode.patch.txt --]
[-- Type: text/plain, Size: 3255 bytes --]

Index: cgen/decode.scm
===================================================================
RCS file: /cvs/src/src/cgen/decode.scm,v
retrieving revision 1.9
diff -c -p -r1.9 decode.scm
*** cgen/decode.scm	21 Oct 2003 16:42:00 -0000	1.9
--- cgen/decode.scm	28 Jan 2004 18:15:11 -0000
***************
*** 1,5 ****
  ; Application independent decoder support.
! ; Copyright (C) 2000 Red Hat, Inc.
  ; This file is part of CGEN.
  ;
  ; This file provides utilities for building instruction set decoders.
--- 1,5 ----
  ; Application independent decoder support.
! ; Copyright (C) 2000, 2004 Red Hat, Inc.
  ; This file is part of CGEN.
  ;
  ; This file provides utilities for building instruction set decoders.
***************
*** 433,462 ****
    (letrec ((opcode (insn-value insn))
  	   (insn-len (insn-base-mask-length insn))
  	   (decode-len (length bitnums))
! 	   (compute (lambda (val insn-len decode-len bl)
  		      ;(display (list val insn-len decode-len bl)) (newline)
  		      ; Oh My God.  This isn't tail recursive.
  		      (if (null? bl)
  			  0
! 			  (+ (if (> (car bl) insn-len)
! 				 0
! 				 (if (bit-set? val
! 					       (if lsb0?
! 						   (car bl)
! 						   (- insn-len (car bl) 1)))
! 				     (integer-expt 2 (- (length bl) 1))
! 				     0))
! 			     (compute val insn-len decode-len (cdr bl)))))))
!     (let* ((opcode (compute (insn-value insn) insn-len decode-len bitnums))
! 	   (opcode-mask (compute (insn-base-mask insn) insn-len decode-len bitnums))
  	   (indices (missing-bit-indices opcode-mask (- (integer-expt 2 decode-len) 1))))
        (logit 3 "insn =" (obj:name insn)
! 	     " insn-value=" (insn-value insn)
! 	     " insn-base-mask=" (insn-base-mask insn)
  	     " insn-len=" insn-len
  	     " decode-len=" decode-len
! 	     " opcode=" opcode
! 	     " opcode-mask=" opcode-mask
  	     " indices=" indices "\n")
        (map (lambda (index) (+ opcode index)) indices)))
  )
--- 433,462 ----
    (letrec ((opcode (insn-value insn))
  	   (insn-len (insn-base-mask-length insn))
  	   (decode-len (length bitnums))
! 	   (compute (lambda (val insn-len decode-len bl default)
  		      ;(display (list val insn-len decode-len bl)) (newline)
  		      ; Oh My God.  This isn't tail recursive.
  		      (if (null? bl)
  			  0
! 			  (+ (if (or (and (>= (car bl) insn-len) (= default 1))
! 				     (and (< (car bl) insn-len)
! 					  (bit-set? val
! 						    (if lsb0?
! 							(car bl)
! 							(- insn-len (car bl) 1)))))
! 				 (integer-expt 2 (- (length bl) 1))
! 				 0)
! 			     (compute val insn-len decode-len (cdr bl) default))))))
!     (let* ((opcode (compute (insn-value insn) insn-len decode-len bitnums 0))
! 	   (opcode-mask (compute (insn-base-mask insn) insn-len decode-len bitnums 1))
  	   (indices (missing-bit-indices opcode-mask (- (integer-expt 2 decode-len) 1))))
        (logit 3 "insn =" (obj:name insn)
! 	     " insn-value=" (number->hex (insn-value insn))
! 	     " insn-base-mask=" (number->hex (insn-base-mask insn))
  	     " insn-len=" insn-len
  	     " decode-len=" decode-len
! 	     " opcode=" (number->hex opcode)
! 	     " opcode-mask=" (number->hex opcode-mask)
  	     " indices=" indices "\n")
        (map (lambda (index) (+ opcode index)) indices)))
  )

Re: [patch][rfa] -opcode-slots: Handling of short insns

[Ben Elliston]

Dave Brolley <brolley@redhat.com> writes:

> > This patch corrects the problem encountered with my internal port. I
> > know of no other port which is affected by this bug. I have tested
> > it against frv and xstormy16 and verified no changes to the
> > generated decoders.
> >
> > OK to commit?

This looks fine and, as you say, you've tested it.

Ben

Re: [patch][rfa] -opcode-slots: Handling of short insns

[Dave Brolley]

Committed.

Ben Elliston wrote:

>Dave Brolley <brolley@redhat.com> writes:
>
>  
>
>>>This patch corrects the problem encountered with my internal port. I
>>>know of no other port which is affected by this bug. I have tested
>>>it against frv and xstormy16 and verified no changes to the
>>>generated decoders.
>>>
>>>OK to commit?
>>>      
>>>
>
>This looks fine and, as you say, you've tested it.
>
>Ben
>
>  
>