search

sy2002 / QNICE-FPGA

QNICE-FPGA is a 16-bit computer system for recreational programming built as a fully-fledged System-on-a-Chip in portable VHDL.
http://qnice-fpga.com
Other
69 stars 16 forks source link

Bug in emulator: MOVE clears Carry and Overflow flags. #37

Closed MJoergen closed 4 years ago

MJoergen commented 4 years ago

The source code of the emulator seems to suggest that MOVE instructions should not affect the C (carry) and V (overflow) flags. This is implied from this line of code in qnice.c:

update_status_bits(destination, destination, destination, DO_NOT_MODIFY_CARRY | DO_NOT_MODIFY_OVERFLOW);

However, the MOVE instruction DOES actually modify the C and V flags in that it clears them. This can be most easily seen by running the following sequence of instructions:

MOVE    0x8765, R0
ADD     0x9876, R0
; Now C and V are set
MOVE    0x0000, R0
; Now C and V have been cleared again

The document qnice_intro.pdf does not give any specific details other than words like "last result" and "last operation". I haven't found any more detailed documentation regarding which instructions affect which status flags, so it's not clear to me what the correct behaviour is, but the source code of the emulator is misleading.

Note: This is seen in the latest develop branch.

bernd-ulmann commented 4 years ago

If I remember correctly, MOVE should leave C and V untouched as these conditions can not result from a MOVE.

sy2002 commented 4 years ago

@bernd-ulmann Emulator fixes (in contrast to Monitor fixes) go to branch dev-io-addr please, because there you have the newest Emulator.

So: Emulator: dev-io-addr Monitor: dev-int

P.S. We have to many loose ends, it is very confusing all these branches 😱 🤯 I do look forward to merge all that stuff back so that we have develop . Makes it much easier :-) But this will take some more weeks due to time constraints on my side.

bernd-ulmann commented 4 years ago

It really confuses me, too... ;-) I just ruined the debug_library.asm and had to restore it as I was in the wrong branch... :-) ARGL

Concerning the MOVE question above: The emulator does it right, I think, it is the hardware that should behave differently. :-)

bernd-ulmann commented 4 years ago

@sy2002 : Should we setup a Zoom meeting for a joint-merge-session soon? :-)

sy2002 commented 4 years ago

@bernd-ulmann re joint merge session: Yes, for sure. Will love to! :-) But only afer I am done with the implementation of the interrupt system and my remaining TODOs in the dev-io-addr branchc :-) (After all, there is a reason, why we have dev-io-addr: It broke so many things that need to be fixed first - by the way there seems to be an emu bug in issue #32 , that needs to be tackled inside the branch dev-io-addr)

sy2002 commented 4 years ago

Concerning the MOVE question above: The emulator does it right, I think, it is the hardware that should behave differently. :-)

@bernd-ulmann Now I am confused ;-) As far as I understood, Michael did not, yet test on hardware. He tested on the emulator. It was a question to the QNICE ISE inventor - you - about what is the correct behaviour... As your answer is buried between separate topics just to make clear that I got it:

Above you write

If I remember correctly, MOVE should leave C and V untouched as these conditions can not result from a MOVE.

Michael writes "MOVE clears Carry and Overflow flags"

So if MOVE should leave C and V untouched as you write and Michael writes that the emulator is actually clearing Carry and Overflow: Isn't there a contradiction to your next sentece (some comments later):

Concerning the MOVE question above: The emulator does it right, I think, it is the hardware that should behave differently. :-)

bernd-ulmann commented 4 years ago

MOVE should leave C and V untouched but you are right, the emulator has a bug here as it clears both SR bits. I will take care of this. :-)

sy2002 commented 4 years ago

@bernd-ulmann Please use branch dev-io-addr for emulator updates.

bernd-ulmann commented 4 years ago

The bit mask which determines which bits of the SR are to be retained was erroneous in the emulator. This has now been fixed as this test program from Michael shows:

000001 .org 0 000002 0000 0F80 8765 MOVE 0x8765, R0 000003 0002 1F80 9876 ADD 0x9876, R0 000004 ; Now C and V are set 000005 0004 E000 halt 000006 0005 0F80 0000 MOVE 0x0000, R0 000007 ; Now C and V have been cleared again 000008 0007 E000 halt

➜ emulator git:(dev-io-addr) ✗ qnice Q> load move.out Q> run 0 HALT instruction executed at address 0004.

Q> rdump Register dump: BANK = 00, SR = VC_1

R00-R03: 1fdb 0000 0000 0000
R04-R07: 0000 0000 0000 0000
R08-R11: 0000 0000 0000 0000
R12-R15: 0000 0000 0025 0005

Q> run 5 HALT instruction executed at address 0007.

Q> rdump Register dump: BANK = 00, SR = __V_ZC_1

R00-R03: 0000 0000 0000 0000
R04-R07: 0000 0000 0000 0000
R08-R11: 0000 0000 0000 0000
R12-R15: 0000 0000 002d 0008

Q> exit

The C- and V-bits are now left unchanged by the MOVE instruction, just the Z-flag is set by the MOVE 0x0000, ...

MJoergen commented 4 years ago

@bernd-ulmann Just one more question: In the emulator, the CMP instruction deliberately always clears the C and X bits of the status register. Is this the intended behavior ? Is the behavior of the status bits documented anywhere (in detail) ?

Furthermore, the function update_status_bits() checks for DO_NOT_MODIFY_X, but this flag is never used in the emulator. This may just be some left-over code, but since I'm writing the CPU function test suite, it's nice to know what the "correct" behavior is. So far, I'm relying on the emulator as the de-facto correct behavior, and using its source code when I get in doubt.

bernd-ulmann commented 4 years ago

As before, this was due to an erroneous bit mask in the CMP-instruction. CMP should now (I am a bit tired, so please bear with me if I made a mistake) preserve the C and X flags in the SR.

P.S.: Done in dev-io-addr branch.

bernd-ulmann commented 4 years ago

The idea of the X-bit comes from the 74LS181 bit slice ALU chips which I used in QNICE's predecessor, NICE (which was a 32 bit machine built in the early/mid 1990s by a friend of mine and me). If memory serves me right, the DO_NOT_MODIFY_X was included in update_status_bits(...) for orthogonality reasons although no operation actually uses this flag.

sy2002 commented 4 years ago

@MJoergen Bernd fixed it on the dev-io-addr branch and closed this one. But as he wrote "I am a bit tired, so please bear with me if I made a mistake" I thought, I would reopen it and assign it back to you for testing.

MJoergen commented 4 years ago

Using the dev-io-addr branch together with the file cpu_test.asm from the dev-cputtest branch shows that this fix is broken. Now the problem is that the ADD instruction fails to clear any previously set carry flag.

MOVE    0x00ff, R14
; Now the carry flag is set
MOVE    0x0000, R0
ADD     0x0000, R0
; Now the carry flag should be clear

So this makes me think that - since the emulator is updated in the dev-io-addr branch - that perhaps the cpu_test.asm file should be committed to this same branch too ? That way, @bernd-ulmann can verify fixes without waiting for me.

sy2002 commented 4 years ago

@bernd-ulmann Reassigned to you for fixing in the dev-io-addr branch. @MJoergen Yes, I guess we should do the cpu_test.asm inside the dev-io-addr branch and delete (or not use any more for now) the dev-cputest branch.

MJoergen commented 4 years ago

Done. I've now pushed the latest cpu_test.asm (from dev-cputest) to the dev-io-addr branch, and will continue development there. The dev-cputest branch can safely be deleted now.

MJoergen commented 4 years ago

Verified and closed.