INCRB, DECRB - Githubissues

bernd-ulmann commented 4 years ago

Hi Mirko, hi Michael - I have something to discuss with you. :-) Michaels question regarding ADD 0x0100, SR made me think... The behaviour that the SR condition flags are cleared after this instruction is OK and intended but...

...as I showed Mirko a couple of days ago, I (personally) like to "return" error conditions or the like from subroutines by setting/clearing certain error flags. I do this often in my Z80 code which has the advantage that one can check for an error condition after calling some subroutine with a simple JR ..., C or the like.

This is basically not possible with QNICE's register bank switching scheme. That is not a real problem but - not nice, not even quite nice. ;-) So I would like to ask you about your opinion regarding the following idea:

We could introduce two additional control instructions (opcode E): INCRB and DECRB. These would increment/decrement only the upper eight bits of SR without affecting the condition bits in the SR.

We could even use one of the two spare condition bits, name it R and set it if an INCRB/DECRB resulted in a wrap around of the register bank address.

What do you think? Should we implement this or just live with ADD 0x0100, SR and just not return condition bits from subroutines?

See you tonight in the phone conference, yours, Bernd. :-)

sy2002 commented 4 years ago

Hi Bernd, cool idea. And I am always pretty relaxed, when we introduce non breaking changes: So did I get it right, when it comes to non-breaking existing code: We just would need to remove the INCRB DECRB macros from our sysdef.asm and then everything should just work, right?

bernd-ulmann commented 4 years ago

Yes, that's right! :-) I will then go ahead and implement INCRB and DECRB in the emulator and update the documentation. :-)

sy2002 commented 4 years ago

And please in the assembler, too ;-)

bernd-ulmann commented 4 years ago

Of course... ;-) ...and the disassembler(s)...

MJoergen commented 4 years ago

I like your idea, but I don't like introducing the new status bit R. I would prefer to use the existing status bits Z, N, and X to denote the result of the INCRB/DECRB being 0, negative, or 0xFF.

bernd-ulmann commented 4 years ago

It's great that we can discuss such topic here. :-) I am not too keen of R and wouldn't have a problem not having it at all. I would, nevertheless, prefer to have the condition bits in the SR unaffected by INCRB/DECRB so that one could retain C and V from a subroutine call. What do you think?

MJoergen commented 4 years ago

Sorry, I was too quick. I agree that the INCRB and DECRB should not modify the status bits. After all, that is the purpose of introducing these two new instructions.

However, I see no real need for a flag to indicate overflow/wraparound. In the register bank test, I just explicitly test for the bank being equal to zero. I think we should remove the R flag from this proposal.

bernd-ulmann commented 4 years ago

OK! I am happy with that! R will be gone in a tick. :-)

bernd-ulmann commented 4 years ago

The emulator now supports INCRB and DECRB.
The builtin disassmbler of the emulator also handles these two new instructions.
The disassembler in the monitor also knows about INCRB and DECRB.
sysdef.asm no longer defines these as macros ADD/SUB 0x0100, SR.
The programming_card- and intro-PDF files have been extended accordingly.
The assembler also takes care of these two new instructions.
The emulator now also displays the address of the last instruction executed at the "Q>" prompt for convenience.

I did some testing including q-tris (I am and will ever be a lousy player :-) ), everything seems to work quite fine. :-)

sy2002 commented 4 years ago

OK starting to implement that in hardware now.

P.S. @bernd-ulmann Just for the record for doing the hardware right: The "old way" of changing SR manually will still work. The old way "just" destroys the SR while the "new way" does not destroy the SR, right?

bernd-ulmann commented 4 years ago

Yes, an ADD/SUB 0x0100, SR will still work. The only difference to INCRB/DECRB is that these two instruction do not affect any condition bits in the SR. :-) Please note that I commented out the INCRB/DECRB macros from sysdef.asm!

bernd-ulmann commented 4 years ago

Hi Mirko - I assigned this to you so that you can complete it with your hardware implementation of these two instructions. :-)

sy2002 commented 4 years ago

Hey Bernd, this thing here is awesome: I did the math and compared before/after: The Monitor alone saves 156 (!) words with the new INCRB/DECRB commands because these are 1-word-commands compared to the old ADD which needed two words.

sy2002 commented 4 years ago

OK - hardware is now supporting this, too: I did my favorite "three-way-multitasking" test, i.e. load Q-TRIS but do not start it yet, load timer-test, then start both and have the "dual screen setup" UART plus VGA and enjoy Q-TRIS, plus seeing the ball on the top line go back and forth and see the messages tick on UART: HARDWARE WORKS. 👍 🥳

I also did run monitor/compile_and_distribute.sh, so that #include ../dist_kit/sysdef.asm includes the correct sysdef.asm (without #define INCRB ...).

@MJoergen Assembler, Emulator and Hardware are supporting the new INCRB / DECRB commands. You might want to adjust your cpu_test.asm so that instead of doing ADD 0x0100, R14 and SUB 0x0100, R14 to change the bank you are using the new CPU commands INCRB and DECRB. So I assigned this to you.

Furthremore: Do you also deliberately test in cpu_test.asm if being in bank FF and doing an INCRB wraps to 00 and being in bank 00 and doing a DECRB wraps to FF? I am asking, because I do not explicitly check that in VHDL but trust that the synthesizer is getting what I mean. Hopefully you find this code correct in regards to make sure the wrap around works? I thought so it will due to (15 downto 8):

                  -- increment the register bank address by one and leave the SR alone while doing so
                  when ctrlINCRB =>
                     fsmSR(15 downto 8) <= SR(15 downto 8) + 1;
                     fsmCPUAddr <= PC;
                     fsmNextCpuState <= cs_fetch;

                  -- decrement the register bank address by one and leave the SR alone while doing so
                  when ctrlDECRB =>
                     fsmSR(15 downto 8) <= SR(15 downto 8) - 1;
                     fsmCPUAddr <= PC;
                     fsmNextCpuState <= cs_fetch;

But even if this is correct: I do foresee open source synthesizers being not as mature as Xilinx and therefore it might make sense that the cpu_test.asm checks the correct wrap around behaviour.

MJoergen commented 4 years ago

Wait a minute! Is this change really necessary?

If R14 contains the vale 0x0005 (i.e. bank 0 with Carry set) and we execute the instruction ADD 0x0100, R14, then the new value will be 0x0105, i.e. carry still set.

I'm on my phone right now, so I can't test it.

sy2002 commented 4 years ago

@MJoergen do you mean by

Is this change really necessary

the two new operands INCRB and DECRB that we introduced today?

Well - they are here now and work with the assembler, the emulator as well as the hardware 😃

And they are nicely saving us precious memory, see my comment above, that I quote here

Hey Bernd, this thing here is awesome: I did the math and compared before/after: The Monitor alone saves 156 (!) words with the new INCRB/DECRB commands because these are 1-word-commands compared to the old ADD which needed two words.

They also speed up performance, because INCRB costs only 2 processor cycles while ADD 0x0100, R14 costs us 4 cycles. Due to the fact that INCRB is our common subroutine "intro" code (instead of using the stack), this performance gain might sum up quite nicely (qnicely ;-)).

So, yet - I guess this is a good thing.

But the cpu_test.asm is not yet testing the right functioning of INCRB and DECRB, which I meant by my comment https://github.com/sy2002/QNICE-FPGA/issues/61#issuecomment-674405153

MJoergen commented 4 years ago

We determined that these new instructions are not strictly necessary, since they behave the same as ADD/SUB 0x0100, R14. However, we'll leave them in for the following two reasons:

They are an optimization, in that they are shorter (one word instead of two words) and therefore faster to execute too.
We've already implemented it.

The cpu_test.asm has now been updated too, and therefore closing this issue.

sy2002 / QNICE-FPGA

INCRB, DECRB #61