Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISA change: New behaviour for the X flag #165

Open
5 of 8 tasks
sy2002 opened this issue Oct 13, 2020 · 13 comments
Open
5 of 8 tasks

ISA change: New behaviour for the X flag #165

sy2002 opened this issue Oct 13, 2020 · 13 comments
Assignees
Labels

Comments

@sy2002
Copy link
Owner

sy2002 commented Oct 13, 2020

First of all, the following decision has to be made:

  1. Shall the X flag act as Bernd described it here: Math Lib in Monitor: Add SHL32 and SHR32 #160 (comment)

  2. Or shall the X-flag act as Michael described it here: Math Lib in Monitor: Add SHL32 and SHR32 #160 (comment)

After that has been decided (@bernd-ulmann), the following TODOs need to be done.

  • Change the intro document
  • Change the programming card
  • Change the emulator
  • Change the CPU
  • Change cpu_test.asm
  • Write a section about our recommended flag handling in our programming best practices
  • Ask Volker if this change leads to any change in VASM or the VBCC toolchain (did he use X?)
  • Do a smoke test on the VBCC apps (e.g. in qbin) in the emulator and on hardware

@MJoergen and @bernd-ulmann: Is this list complete?

@MJoergen
Copy link
Collaborator

That list seems complete.

@MJoergen
Copy link
Collaborator

As mentioned in #160 (comment) I support @bernd-ulmann 's proposal for the X-flag.

@bernd-ulmann
Copy link
Collaborator

I have already changed the following things:

  • Emulator now changes the X bit only implicitly in the case of SHL/SHR.
  • Into and programming card have been updated.

MJoergen added a commit that referenced this issue Oct 14, 2020
@MJoergen
Copy link
Collaborator

I've updated the cpu_test.asm so it now passes with the new emulator.

I've done a smoke-test of our demo programs on the emulator.

MJoergen added a commit that referenced this issue Oct 14, 2020
@MJoergen
Copy link
Collaborator

Fixed and verified in hardware.

@sy2002
Copy link
Owner Author

sy2002 commented Oct 14, 2020

Great job, both of you gentlemen! And speedy ;-)

@bernd-ulmann Could you rename all ISA version references (is it just the headline in the intro.pdf or are there more) to V1.7? This ISA change leads to a new ISA version number...

@MJoergen
Copy link
Collaborator

I still need to update the MTH$SHR32 routine so that it actually does shift into X.

@MJoergen
Copy link
Collaborator

Actually, the routine already worked :-) I've updated the test program test_shift.asm to test this behaviour.

NOTE: In the current implementation, the routine SHR32 correctly sets the X flag following the same semantics as the SHR instruction. However, the value of the C flag after the call is undefined.

Likewise the routine SHL32 correct sets the C flag as expected, but the value of the X flag after the call is undefined.

This is done to simplify the implementation. This is a (slight) difference from the semantics of the SHL and SHR instructions. I added a short comment about this in math_library.asm.

I hope this is ok ?

@sy2002
Copy link
Owner Author

sy2002 commented Oct 15, 2020

@MJoergen For me it is absolutely OK.

Though it "feels" like a slight inconsistency. As I am currently in a rush I did not look at the code so a quick question to you: Save the X or the C flag before the routine and restoring it on exit for not having it undefined but as it was on entry: Would this break the elegance of the implementation?

@MJoergen
Copy link
Collaborator

I don't know about "elegance" :-D But it would require some more instructions, and I'm just asking whether it is worth it? I mean, would anyone use it?

The tricky part is to restore the X flag without destroying the newly generated C flag (or vice versa). So several instructions must be used to mask the particular status flag to be copied.

@bernd-ulmann
Copy link
Collaborator

Great job, both of you gentlemen! And speedy ;-)

@bernd-ulmann Could you rename all ISA version references (is it just the headline in the intro.pdf or are there more) to V1.7? This ISA change leads to a new ISA version number...

Done. :-)

@sy2002
Copy link
Owner Author

sy2002 commented Oct 17, 2020

When doing my initial grep to check the X register usage, I did only grep for the positive X, but not for the negative !X. So doing a grep -i ", \!X" *.asm (note the !X) revealed in test_programs these files:

qtransfer.asm
sdcard.asm
vga_int.asm

and in montior these files:

fat32_library.asm
qtransfer.asm

@MJoergen I will take care of my sources, i.e. all but vga_int.asm. There it looks like you are using X to check for -1? Please check (and maybe fix) this here:

https://github.com/sy2002/QNICE-FPGA/blob/develop/test_programs/vga_int.asm#L148

_ISR_1              MOVE    @R2, R3
                    MOVE    R6, @R2
                    MOVE    R3, R6
                    SUB     1, @R0
                    RBRA    _ISR_1, !X

@MJoergen
Copy link
Collaborator

Fixed and verified on hardware

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants