search

open-simh / simh

The Open SIMH simulators package
https://opensimh.org/
Other
487 stars 93 forks source link

SDS shifts limit count to 48; sds_cpu.c doesn't for cycle/rotate shifts #315

Open at7heb opened 1 year ago

at7heb commented 1 year ago

The SDS Technical Reference manuals say that if the shift count is > 48, it is set to 48. The LSH and RSH implementations in sds_cpu.c do that for logical, arithmetic shifts, and normalize. For circular shifts, the shift count is incorrectly reduced modulo 48.

I corresponded with Mark Emmer, a colleague from NOAA/DOCB. He verified this with the logic equations in the 900097A-Volume 2 Examiner Diagnostic Manual, volume 2, e.g. 10/1964 edition page 176, left cycle, phase 1, T2, where x48 is set to = not(not(s6) not(s7) not(s8)) + s9 * s10 (meaning if count has the 32 and 16 bits set, or any of the 64, 128, or 256 bits set.

Here's the code for left cycle/rotate

        case 04:                                        /* left cycle */
            sc = sc % 48;                               /* mod 48 */
            if (sc)                                     /* rotate */
                RotR48 (48 - sc);
            break;

IMO, the shift count should be adjusted at the beginning of the LSH and RSH functions, as the 930/940 do.

PS: curiously, sds_cpu.c also has code for a rotate left normalize. The documented normalize shifts zeros into the LSB of the B register as it normalizes. The code in sds_cpu.c shifts sign bits of A into the LSB of B in the undocumented variant. Any insights?

markpizz commented 1 year ago

@rms47 and @kenrector thoughts on this?

rms47 commented 1 year ago

Yes, he's right. It's a bug. Shift counts should be limited to 48 at the top of LSH and RSH. Further, rotates of 48 are effectively NOPs and should be detected before calling RRot48.

The rotate left normalize falls out of the logic equations for left shift. The 940 doesn't explicitly test for unspecified shift combinations; the Tech Manual says they can be ignored because they are illegal. The combination of normalize and cycle is at least predictable, based on the logic equations. I can't figure out what the undefined right shift combinations would do.

I've posted the updated source to my web site.

at7heb commented 4 months ago

Yes, he's right. It's a bug. Shift counts should be limited to 48 at the top of LSH and RSH. Further, rotates of 48 are effectively NOPs and should be detected before calling RRot48.

The rotate left normalize falls out of the logic equations for left shift. The 940 doesn't explicitly test for unspecified shift combinations; the Tech Manual says they can be ignored because they are illegal. The combination of normalize and cycle is at least predictable, based on the logic equations. I can't figure out what the undefined right shift combinations would do.

I've posted the updated source to my web site.

Assign me if you wish; I'll be submitting a pull request presently. Mark Emmer pointed out that this repository was (the only one) missing the fix.

rms47 commented 4 months ago

Sigh... This has been fixed, along with numerous other bugs, in the current version of 3.X for months. See simh.trailing-edge.com/sources/current. The entire directory needs to be merged into "Supnik Current" and then forward merged into mainline. To make this easier, I've zipped it up and released it as SimH v3.12-5.