Update pdp8_cpu.c Rewrite of instruction decoder, implement Major States

[poetnerd]

…tates.

• Update pdp8_cpu.c

Rewrite of instruction decoder to implement Major States.

• Update pdp8_cpu.c

Resolve issues with some C compilers

[bscottm]

@poetnerd: Check line 409 in pdp8_cpu.c. You're probably missing an =. (Note on your discussion with Pizz: Despite the makefile turning on C99 for the GNU C compiler, there are at least a few people who compile with much older compilers that don't catch certain coding issues and don't allow variable declarations anywhere.)

[poetnerd]

Re: line 409: Wow! You're right. I need to reach out to the person who contributed that line and confirm it's a test, and that we've not been leaning on the fact that the assignment at that line will be unconditionally true.

Re: Many compiler issues: Understood. Pizz was right, and I made the changes before submitting here.

I'll be back with an update asap.

[poetnerd]

Ok. I've verified with the original author of that section that the comparison was indeed a typo, and that we're not relying on the unconditional evaluation of the assignment.
That person clarified that this change is necessary to get TSS/8 booting.

[poetnerd]

I'm still a noob with regards to using the ghithub pull request workflow.
I've made my update in my fork and checked that in.
I see that it appears in this thread.
Is there anything else I need to do to move this forward?

[markpizz]

Actually, I reviewed your original issue about this and I have a few questions:

1. The original pdp8 code simulated instructions and was never intended to be cycle accurate. The presumption now is that the simulator is cycle accurate.
   a) Is that true?
   b) if so, must this cycle accurate mode be explicitly enabled. If so, how? If not, and thus the simulator is now just simulating cycles, then the updated PDP8 simulator should change some SCP global variables. Specifically:

extern const char *sim_vm_interval_units;               /* Simulator can change this - default "instructions" */sim_
extern const char *sim_vm_step_unit;                    /* Simulator can change this - default "instruction" */

2. The SCP STEP command steps a particular number of cycles, right? Then sim_vm_step_unit should point to "cycle".

Are these reasonable questions?

[pmetzger]

@markpizz You forked this codebase and make all your changes closed source. Why don't you stay working on that instead of pretending that you are part of OpenSIMH or trying to lure people into using your closed source fork?

[vrs42]

@markpizz You forked this codebase and make all your changes closed source. Why don't you stay working on that instead of pretending that you are part of OpenSIMH or trying to lure people into using your closed source fork?

Please don't do that.

Just because @markpizz hasn't always done things in the way you and I felt was most helpful, does not make it sensible to refuse his help when he is actually helpful, which is a fair bit of the time. More bad feelings just wastes all our time.

[poetnerd]

Hi Mark,

Before I try to answer your questions I want to clarify a bit about what "cycle-accurate" means here in the PDP-8 code. Indeed I characterized my pull request as, "Adding support for Major States" because of what it means to be cycle-accurate in a PDP-8.

Depending on how familiar you are with the PDP-8 architecture, I may not be telling you anything you don't already know. I apologize in advance for that.

The answer to your question, "Is this cycle-accurate or instruction-accurate?" is both straightforward and complicated:

Straightforward answer: Yes. Unconditionally Cycle-Accurate now.

Complicated answer: We chose to do it this way after careful review of the PDP-8 architecture, fixing a bug in the existing code, making the emulation more efficient, and recognizing that a run-time check was a significant performance hit, whereas adding cycle-accurate emulation enabled support of the widely offered "Single Step" action.

I will go back to the guys who wrote the cycle-accurate code and see what they have to say about the SCP global variables. It may be that they left them alone because of how Major States end up not affecting all instructions. I'll get back to you on that.

Meanwhile here is detail:

Cycle accurate emulation means that we now do have finer granularity within within some but not all instructions.

The 3 PDP-8 Major States are "Fetch", "Defer", and "Execute", and the "Execute" state is a bit of a misnomer. Within the three classes of PDP-8 instrutions, "Operate (OPR)", "Input/Output (IOT)", and "Memory Reference (MRI)", not all Major States are visited.

OPR and IOT instructions all complete within the Fetch major state. This means that only MRI instructions ever get to one or both of the other Major States. (You'd think they'd do Fetch the Execute, but no. It's all done in Fetch.

MRI instructions that don't use indirect addressing i.e. they don't perform a second fetch from memory for the destination address, and the JMS (JuMp to Subroutine), just do Fetch then Execute.

The Defer Major State is used for that second memory fetch for Indirect addressing, and to store the return address in JMS.

Before making this pull request, I did a lot of benchmarking, and digging in to understand how the instructions were emulated. The new version of pdp8-cpu.c actually makes a more efficient emulation even before adding in the extra inner loop to track Fetch, Defer, and Execute.

Additionally we discovered that some of the existing instruction state tracking was incomplete, and we added that in. This fix you see, I believe, at line 446 where we update MB. That additional assignmnet, all by itself cost 15% performance, but properly records the right state in the MB register.

This also means that, an additional run-time test for Cycle Accurate versus Instruction Accurate operation is an immediate 15% or more performance hit for all emulation.

Because OPR and IOT instructions always complete in the Fetch state, they operate at about the same speed with the new code as with the old code.

All this boiled down to the decision to offer a change that switched from always instruction-accurate to always cycle-accurate emulation.

The performance was pretty close to original, but now you actually can implement "Single Step" as nearly all members of the "Family of 8" offer.

Yes, this code is a shift always to be cycle accurate. It turns out the instruciton emulation is already so efficient, that the test for a run-time check of cycle-accurate versus instruction-accurate incurs a 15-30% performance hit! I spent a lot of time understanding the differing approaches to the instruction emulation

[markpizz]

All of that makes sense and is fine. Setting the mentioned global variables to "cycles" and "cycle" merely affects the output of some SCP core commands you can enter at the sim> prompt. Most are related to some aspect of timing or throttling.

It is fine that your group carefully considered the simulation performance impact of various parts of the changes you were making, but as I mentioned in the original where you described this project, as a practical matter, as long as simulators running on modern processors can execute instructions (or cycles) many times faster than the original hardware there everyone will be fine with the results.

One thing that the change from instructions to cycles will actually affect is how the simulation framework (SCP) schedules simulated device activities. Whenever a simulated device wants the framework to come back later to finish an I/O operation it will be calling sim_activate (uptr, NNNN), where NNN is the number of instructions (now cycles). The changes you've made from instructions to cycles will affect when these device callbacks (aka activations) happen. The result will be that fewer instructions will now get to execute before the respective callbacks. The consequences will likely not be very significant, but there may be cases where the NNN values should probably be adjusted to somewhat larger values.

[pkoning2]

As I said in the discussion for issue #434:

The question to me is what problem needs to be solved. Open SIMH is, fundamentally, an instruction emulator. It emulates instructions accurately enough that essentially all existing (and new, for that matter) code works correctly. But it does not aim to expose internal hardware details of specific implementations that are not visible from the programmer's point of view.

It seems to me that major state emulation is an example of a hardware detail not visible to the programmer. Here I am discounting the use of the "step single cycle" console panel switch, which is a service technician tool, not something a programmer would use. And in fact, I don't think I have ever seen an FE use it, either -- they tend to run diagnostics and, on older machines, poke around on suspect boards with their handy oscilloscope.

So the answer would be, yes it can be done, at a cost in performance, but why would we do it?

[pmetzger]

@vrs42:

Just because @markpizz hasn't always done things in the way you and I felt was most helpful, does not make it sensible to refuse his help when he is actually helpful, which is a fair bit of the time. More bad feelings just wastes all our time.

I don't particularly love the fact that he's free to take whatever changes we make here to improve SIMH and put them in his closed-source fork, and I don't like that he encourages people submitting bug reports to use his fork instead of the main SIMH.

[pkoning2]

Perry, those are the rules of BSD-style licensed open source software. The answer would be different if Open SIMH used GPL, but we don't and won't. With our license, as for any BSD style license, anyone is free to make a closed fork of our work.
But let's not discuss that further. The subject here should be what goes into Open SIMH.

[poetnerd]

I've taken a stab at answering the question @pkoning2 asked in the discussion for issue #434.

QUESTION: Do we want to have that conversation there in that issue or here in this issue?

[bscottm]

@poetnerd: "Cycle accurate" can mean a few different things. @markpizz interprets it as accurate in instructions/second or instruction cycles/second. There's the sense in which you're using it, namely, machine cycle accurate, i.e., simulating a complete machine cycle in the way that more deeply matches the way the original hardware worked.

I don't think you need to delve into making the PDP-8 simulator instructions/second accurate or make that the default setting.

[rcornwell]

The problem I have with this PR, is that it is not really cycle accurate. It is missing three cycle break. And devices have not been modified to send data one word at a time. Also EAE has no cycles. The only point I can see of this change is to drive the PiDP8 LED's closer to actual hardware.

[poetnerd]

Hello @rcornwell ,

You raise a good point. Notice tht the PR is for "Adding Major States", and that's partly to stay away from the paradigm of "cycle-accurate" in keeping with the published principle of OpenSIMH, "instruction-accurate in preference to cycle-accurate".

TBH, I had not dug into how the EAE and the emulated device support had made use of the Major States.

I think it's an interesting question, "Is there value to updating the EAE and IOT code to make fuller use of the Major States?"

My selfish motivation is, "If Major States as offered in this PR is accepted, the PiDP-8/i project has a smaller footprint of our fork.

A Polyanna interpretation of the PR might be, "With Major States available, follow-on work could provide richer device emulation."

I know that someone is also working on a "Family of 8" update to SIMH that would allow run-time setting of "Straight-8" versus "8/i" versus "8/e" versus "VT278" versus "PDP-12" operation. The 8/i and PDP-12 EAE don't have Mode B, and that change might benefit from utilization of Major States as a foundation.

My question to you, Richard, is: Do you think the PR should be rejected as incomplete, or that it could be accepted in the Polyanna interpretation, as serving as a basis for follow-on improvements?

[rcornwell]

I am not sure whether this PR should be rejected or not. However here I way I would reject it.

1. This is not a completely cycle accurate PDP8 emulation.
   a) Lacks support for EAE.
   b) Lacks cycle support for devices.
   c) I don't see any way to enable single cycle operation.

2. The code is overly complex.

3. It does not serve much of a purpose in terms of emulation.

4. The commit message does not fit SimH convention.

On an emulator there is really no need to single cycle stepping (this was for debugging hardware). It also appears to be used specifically for PiDP8 users. When I did support for the PiDP10 there was little changes. Check out patch: rcornwell/sims@e42f503 to see changes for support of the PiDP10 panel.

I have a electrically accurate model of a PDP8e, that will run OS/8 and TSS/8. However there are differences between various PDP8 models that effects the cycles and results. I don't see these differences taken into account here.

[pkoning2]

I posed the question to the Steering Group, and the answer from Bob Supnik (as I understand it) was that this change is not appropriate for the primary emulator. But we allow multiple emulators for a given machine (you can see this in the pdp10 series of emulators, for example) so if you want to submit this as an alternate emulator (perhaps "pdp8-ms") that would be an acceptable contribution.