Re: [patch][rfa]: Decoding insns > 32 bits in length - committed

Re: [patch][rfa]: Decoding insns > 32 bits in length - committed

[Dave Brolley]

Looks like Nickc committed the opcodes part. The cgen part (attached) 
has now been tested on fr30, m32r and several internal ports. It has 
been approved by fche and committed.

Dave

Dave Brolley wrote:

> Frank Ch. Eigler wrote:
>
>> brolley wrote:
>>
>> : [...]
>> : The patch to opcodes/cgen-ibld.in fixes a problem which was affecting
>> : the fr30 port and an internal port I'm working on. The patch is an
>> : obvious improvement, since 'x' is now at least initialized and decodes
>> : correctly for fr30 and the internal port.
>> : [...]
>>
>> Take this up with nickc; he added that "#if 0" about a month ago.
>>
> ok - I will. I suspect that he only meant to #if out the "int big_p = 
> ....." since it is unused.
>
>> : The patch to cgen/utils-gen.scm corrects the length argument in the
>> : generated calls to EXTRACT_MSB0_UINT in <target>-decode.cxx for SID
>> : cpus. [...]  As far as I can tell, this does not affect any other
>> : ports.
>>
>> I wouldn't expect this sort of systemic bug to have gone undetected
>> this long.  Can you spell out a failing case?
>>
> This would only affect architectures which resort to the base-insn + 
> (additional insn words) model for decoding insns. Do we have any of 
> these? The fr30 has 48 bit insns, but is not a SID simulator. The case 
> which bit me was like this:
>
> The architecture has 16 bit insns which may be followed by 1 or two 
> additional immediate words (16 bits each). In the case that the insn 
> was followed by two additional immediates, the decoder was generating
>
>  word_1 = current_cpu->GETIMEMUSI (pc, pc + 2);
>    f_dest_addr_2 = (0|(EXTRACT_MSB0_UINT (word_1, 32, 16, 32) << 0));
>    f_source_addr_1 = (0|(EXTRACT_MSB0_UINT (word_1, 32, 0, 16) << 0));
>
> Note the length for the extract of 'f_dest_addr_2' which should be 16, 
> but is computed as 32. This is because (in utils-gen.scm), end==32 and 
> word-end==32. Therefore word-length gets substituted as the number of 
> bits to extract.
>
> The computed value is supposed to be the number of bits to extract. 
> Now I see that while the patch I made is sufficient for this 
> particular case, the general solution which handles fields which 
> overlap word boundaries is in the attached patch which replaces my 
> previous patch.
>
> Dave
>
>
>------------------------------------------------------------------------
>
>Index: utils-gen.scm
>===================================================================
>RCS file: /cvs/src/src/cgen/utils-gen.scm,v
>retrieving revision 1.5
>diff -c -p -r1.5 utils-gen.scm
>*** utils-gen.scm	2001/01/06 12:11:09	1.5
>--- utils-gen.scm	2001/11/06 18:29:26
>***************
>*** 118,124 ****
>  			   unsigned? lsb0?)
>    ; ??? lsb0?
>    (let ((word-end (+ word-start word-length))
>! 	(end (+ start length)))
>      (string-append "("
>  		   "EXTRACT_"
>  		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
>--- 118,125 ----
>  			   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")
>***************
>*** 137,144 ****
>  				       (- start word-start)))
>  		   ", "
>  		   (number->string (if (< end word-end)
>! 				       (- word-end end)
>! 				       word-length))
>  		   ") << "
>  		   (number->string (if (> end word-end)
>  				       (- end word-end)
>--- 138,145 ----
>  				       (- start word-start)))
>  		   ", "
>  		   (number->string (if (< end word-end)
>! 				       (- end base)
>! 				       (- word-end base)))
>  		   ") << "
>  		   (number->string (if (> end word-end)
>  				       (- end word-end)
>
> word-extract.patch.txt
>
> Content-Type:
>
> text/plain
> Content-Encoding:
>
> 7bit
>
>

Re: [patch][rfa]: Decoding insns > 32 bits in length - committed

[Dave Brolley]

Looks like Nickc committed the opcodes part. The cgen part (attached) 
has now been tested on fr30, m32r and several internal ports. It has 
been approved by fche and committed.

Dave

Dave Brolley wrote:

> Frank Ch. Eigler wrote:
>
>> brolley wrote:
>>
>> : [...]
>> : The patch to opcodes/cgen-ibld.in fixes a problem which was affecting
>> : the fr30 port and an internal port I'm working on. The patch is an
>> : obvious improvement, since 'x' is now at least initialized and decodes
>> : correctly for fr30 and the internal port.
>> : [...]
>>
>> Take this up with nickc; he added that "#if 0" about a month ago.
>>
> ok - I will. I suspect that he only meant to #if out the "int big_p = 
> ....." since it is unused.
>
>> : The patch to cgen/utils-gen.scm corrects the length argument in the
>> : generated calls to EXTRACT_MSB0_UINT in <target>-decode.cxx for SID
>> : cpus. [...]  As far as I can tell, this does not affect any other
>> : ports.
>>
>> I wouldn't expect this sort of systemic bug to have gone undetected
>> this long.  Can you spell out a failing case?
>>
> This would only affect architectures which resort to the base-insn + 
> (additional insn words) model for decoding insns. Do we have any of 
> these? The fr30 has 48 bit insns, but is not a SID simulator. The case 
> which bit me was like this:
>
> The architecture has 16 bit insns which may be followed by 1 or two 
> additional immediate words (16 bits each). In the case that the insn 
> was followed by two additional immediates, the decoder was generating
>
>  word_1 = current_cpu->GETIMEMUSI (pc, pc + 2);
>    f_dest_addr_2 = (0|(EXTRACT_MSB0_UINT (word_1, 32, 16, 32) << 0));
>    f_source_addr_1 = (0|(EXTRACT_MSB0_UINT (word_1, 32, 0, 16) << 0));
>
> Note the length for the extract of 'f_dest_addr_2' which should be 16, 
> but is computed as 32. This is because (in utils-gen.scm), end==32 and 
> word-end==32. Therefore word-length gets substituted as the number of 
> bits to extract.
>
> The computed value is supposed to be the number of bits to extract. 
> Now I see that while the patch I made is sufficient for this 
> particular case, the general solution which handles fields which 
> overlap word boundaries is in the attached patch which replaces my 
> previous patch.
>
> Dave
>
>
>------------------------------------------------------------------------
>
>Index: utils-gen.scm
>===================================================================
>RCS file: /cvs/src/src/cgen/utils-gen.scm,v
>retrieving revision 1.5
>diff -c -p -r1.5 utils-gen.scm
>*** utils-gen.scm	2001/01/06 12:11:09	1.5
>--- utils-gen.scm	2001/11/06 18:29:26
>***************
>*** 118,124 ****
>  			   unsigned? lsb0?)
>    ; ??? lsb0?
>    (let ((word-end (+ word-start word-length))
>! 	(end (+ start length)))
>      (string-append "("
>  		   "EXTRACT_"
>  		   (if (current-arch-insn-lsb0?) "LSB0" "MSB0")
>--- 118,125 ----
>  			   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")
>***************
>*** 137,144 ****
>  				       (- start word-start)))
>  		   ", "
>  		   (number->string (if (< end word-end)
>! 				       (- word-end end)
>! 				       word-length))
>  		   ") << "
>  		   (number->string (if (> end word-end)
>  				       (- end word-end)
>--- 138,145 ----
>  				       (- start word-start)))
>  		   ", "
>  		   (number->string (if (< end word-end)
>! 				       (- end base)
>! 				       (- word-end base)))
>  		   ") << "
>  		   (number->string (if (> end word-end)
>  				       (- end word-end)
>
> word-extract.patch.txt
>
> Content-Type:
>
> text/plain
> Content-Encoding:
>
> 7bit
>
>