VBCC: Test the ISR feature and write a timer interrupt/ISR test for VBCC

[sy2002]

IMPORTANT: As of time of writing, this only works on branch dev-vbcc-vasm-fix

VBCC supports ISRs: Test the ISR feature and write a timer interrupt/ISR test for VBCC and put it to c/test_programs/timer_isr.c

There seems to be a special ISR "tag" or something that you can use in C programs as you an see here:

As soon as Volker anwers us how to use it: We should:

1. Write an appropriate test program
2. Enhance doc/best_practices and explain this feature.

Would be cool, if we would be able to add this still to V1.6, because V1.6 was the version where we introduced interrupts. It would make the release more well-rounded

[sy2002]

Here is the correct C syntax for an ISR:

__interrupt __rbank void irq(void)
{
...
}

So the __interrupt prefix is used to write ISRs. We should also use this issue to document this in the C section of the best practices programming guide

EDIT in October 2020: Using register banks in ISRs is in the meantime absolutely forbidden.

[sy2002]

In commit 56d0596 I updated the documentation about this feature.

[MJoergen]

It seems we need more work on this feature.

Assuming the goal is to be able to write an ISR in C (rather than in assembly), I tried the following:

static unsigned int counter;

static __interrupt __norbank void isr(void)
{
   counter++;
}

So this is intended to be a bare-minimum ISR that simply increments a global counter.

After compiling with the -k switch to investigate the generated assembly code I find:

l2:
   move  #l1,R12
   add   1,@R12
l3:
   rti

...

l1:
   .space   2

The problem here is that this destroys the R12 register. I was expecting the compiler to insert a MOVE 0xFF00, SR instruction and then to make use of registers R0 to R7.

I'm not sure how to proceed, maybe we should have another meeting ?

[sy2002]

@MJoergen You're right: The compiler needs to make sure that it only uses the registers RO to R7 and indeed it needs to stick to our convention of doing the regbank switch via MOVE. By the way, I guess you made a typo, the correct MOVE is MOVE 0xF000, SR according to the best-practice documentation. But maybe you deliberately wanted to use 0xFF00 as the ISR itself would not use regbank switching at all, leaving more regbanks for the regular code?

Also my gut feeling is, that when we or Volker touch the compiler backend, we might want to get rid of the necessity to do a __norbank for ISR: Using MOVE 0xF000, SR instead of INCRB in an ISR is just as much as a convention than the fact, that an ISR needs to end with RTI instead of RET.

So I propose the following new semantics for the compiler:

• An ISR is denoted by __interrupt.
• No matter what the global "rbank" settings (e.g. via compiler switches) are: The compiler never(**) uses register bank switching inside an ISR. Therefore the the need for explicitly mentioning __norbank for ISRs goes away.
• The ISR always ends with RTI instead of RET
• The ISR always beings with MOVE 0xF000, SR.
• The ISR only uses R0 to R7.

What are your thoughts? As soon as we all agree, we could approach Volker.

regarding (**): If the compiler backend would be extremely smart, it would use up to 16 register banks inside an ISR and only when reaching bank 15 and needing more registers would use the stack. But I guess that this would lead to a very complex logic, so I instead phrased this simple rule and said "never uses register bank switching" in an ISR.

[MJoergen]

First of all, when I wrote MOVE 0xFF00, SR that was indeed a typo, and it should have been MOVE 0xF000, SR.

I like your proposal about not using register bank switching in an ISR. This also encourages the programmer to keep the ISR short, since anything complicated will use the stack, which is (somewhat) slower.

However, we should be clear that it the programmer uses e.g. multiplication, that will result in subroutine calls inside the C library, which use register bank switching.

I initially had the idea that we could in fact use MOVE 0xFF00, SR since only one register bank would ever be needed, but because of C library calls, monitor calls, etc, we do need several register banks, so I propose we proceed with @sy2002 's proposal, including MOVE 0xF000, SR.

[sy2002]

However, we should be clear that it the programmer uses e.g. multiplication, that will result in subroutine calls inside the C library, which use register bank switching.

You are right. This is why we need to do MOVE 0xF000, SR.

@MJoergen We also need to make sure to block ISRs inside the arithmetic functions to avoid undefined states of e.g. EAE when the main program is in the middle of a multiplication and then the ISR also wants do do that. Our agreed upon solution was, that we PAUSE/RESUME aka BLOCK/UNBLOCK the hardware interrupts as we defined it here under "Solution for V1.7": https://github.com/sy2002/QNICE-FPGA/issues/138#issuecomment-695349058

And: Please do not forget to implement and test this BLOCK/UNBLOCK as defined here: https://github.com/sy2002/QNICE-FPGA/issues/138#issuecomment-695357988 BTW: I assume that the list mentioned there needs to be enhanced be SHL32/SHR32?

About the compiler: I will approach Volker and CC both of you.

[MJoergen]

@sy2002 @bernd-ulmann : I just thought of something: When in an ISR, we need to save R8-R12 as well. For instance, the MULU monitor call uses these registers.

Would it be a possibility for the CPU to save all the registers R8-R15 in shadow registers (and not just R13-R15) ? That way, there is no need for explicitly saving them to stack.

[bernd-ulmann]

I like the idea of eight shadow registers instead of three (a power of two is way nicer than that strange 3 :-) ). Let me think about it for a moment...

[bernd-ulmann]

Yes, that is really nice. I have changed the emulator accordingly. There are now eight shadow registers which are loaded with R8..R15 in the case of an interrupt and can be accessed by the EXC-instructions. At RTI the contents of these registers are copied back to R8..R15. :-)

I also updated the programming card accordingly (there are no details on the shadow registers in the introductory slides).

[sy2002]

OK I will update the CPU

[MJoergen]

@sy2002 : Maybe we should inform Volker about this change? The way I see it, with these new changes, an ISR can use all registers R0-R12, and not just R0-R7. Actually, is this not what the C compiler already does? So the only change needed in the C compiler is to add the MOVE 0xF000, SR as the first instruction.

[sy2002]

@MJoergen Good point, I will update my email.

[sy2002]

Status:

• I have mailed to Volker
• I have created two tasks for myself to enhance the hardware: issue #172 and issue #173

[MJoergen]

Volker replied by mail and questioned our design. I think perhaps it is time to sit down and re-evaluate. The reason is that with the current compiler interrupt handling does seem to work as it is.

For instance, the following small test program:

static void setPalette(unsigned int addr, unsigned int data)
{  
   unsigned int oldPaletteAddr = MMIO(VGA_PALETTE_ADDR);
   MMIO(VGA_PALETTE_ADDR) = addr;
   MMIO(VGA_PALETTE_DATA) = data;
   MMIO(VGA_PALETTE_ADDR) = oldPaletteAddr;
}  
   
static __interrupt void isr(void)
{  
   static unsigned int next_line = 0;
   
   // Set background colour
   setPalette(VGA_PALETTE_OFFS_USER+16, next_line*67);
   
   // Prepare for next scan line
   if (++next_line == 480)
   {
      next_line = 0;
   }
   MMIO(VGA_SCAN_INT) = next_line;
}

results in the following assembly code (when compiled with -O3):

   move  R0,@--R13
   move  #l7,R11
   move  @R11,R8
   move  65304,R12
   move  R8,@R12++
   move  67,@R12++
   move  65308,R11
   move  0,@R11
   move  @R12,R10
   move  48,R0
   move  65338,R11 
   move  @R11,R9
   move  65338,R12
   move  R0,@R12
   move  65339,R12
   move  R10,@R12
   move  65338,R12 
   move  R9,@R12
   add   1,R8
   move  #l7,R12
   move  R8,@R12
   cmp   480,R8
   rbra  l9,!z
   move  #l7,R12
   move  0,@R12
l9:
   move  #l7,R11
   move  65347,R12
   move  @R11,@R12
   move  @R13++,R0
   rti

Notice how the register R0 gets pushed onto the stack.

This program works in hardware too already, provided the main loop does not use the registers R8-R12. So when the hardware is changed, see Issue #172 (and the monitor, see Issue #138), then the above program should work just fine.

So I'm basically asking, do we really need any interrupt-related changes to the compiler?

The one big thing we talked about was to reserve a few register banks for the ISR, using MOVE 0xF000, SR. I'm not a big fan of this idea (even though it was my own suggestion!!), and I wonder whether it is still needed?

Anyway, these are just my thoughts.

[sy2002]

We have two distinct topics here, because I asked Volker to do two things:

1. Make sure that __interrupt always implicitly works like __interrupt __norbank

2. Adding MOVE 0xF000, SR to the beginning of each ISR

1)

The reason why your above-mentioned static __interrupt void isr(void) works and pushes everything to the stack is because you are telling the VBCC compiler to not use any register banks at all. As described here, you would need to set the option -speed plus set -rw-threshold=0 to enforce register bank switching being enabled.

Having register bank switching on for "normal" (non-interrupt) functions makes a lot of sense. This is why the standard C library is compiled using these options and also qbin/make.shuses it: C_FLAGS_MM="-c99 -O3 -rw-threshold=0 -speed -maxoptpasses=100".

This is why I originally thought it might make sense that __interrupt automatically adds __norbank to the function, so that we have a guaranteed stack based handling, even if the programmer would forget to specify __norbank.

Right now, the best practice document is telling the programmer to do so (to always add __norbank when using __interrupt): Documented here

I am absolutely fine not to ask Volker to change this very behaviour and the programmer "stays responsible" for deciding, if he wants to use register banks and if he does to add __norbank.

BTW (OFF TOPIC): While writing this text, I wonder, if the bugs I described in https://github.com/sy2002/QNICE-FPGA/issues/87#issuecomment-716021287 might be related to the fact that I used the very make file in qbin to build maze2d and you are not having __norbank? (Too tired now to test; but you might want to.)

2)

The MOVE 0xF000, SR is needed as long as we want to allow "TSR" programs (programs that run in the background while other programs run) and as long as we want to stick to the agreement we found a while ago, that the main program shall be allowed to use the register banks not only in a INCRB/INCRB/DECRB/DECRB manner but also in a way that it reserves for example 3 banks and from time to time accesses memory from "below". While doing so, an interrupt with an INCRB would kill this very "below" register-memory:

INCRB
<fill bank with values>
INCRB
<fill next bank, too>
INCRB
<do stuff>
DECRB
DECRB
MOVE R0, R8 ; retrieve one value from the lower bank
INCRB
INCRB
<continue to work>
MOVE R8, R0 ; work with the value that as just been retrieved

Do I remember right that this was the reason we came up with the MOVE 0xF0000 solution, because we did not want to introduce for example a shadow register that is a register-bank-pointer?

[MJoergen]

Re 2) Yes, that is correct. I still think this is a very special use case, and not something that a casual user writing an ISR in C would need. I mean, the C compiler will not by itself generate code that uses the register banks as random access memory. This use case is more relevant when writing programs directly in assembly, in which case the ISR would be written in assembly too, and then the programmer is free to add the MOVE 0xF000, SR instruction by himself.

Re 1) I feel we need some more experience with interrupt routines written in C to understand what is needed (or often used).

[sy2002]

re 1)

As we documented in the best practices, that you should always use __norbank in conjunction with __interrupt, I am fine with leaving VBCC as it is.

re 2)

I mean, the C compiler will not by itself generate code that uses the register banks as random access memory.

You are right. I did not consider this. If you stick to a C-only environment, then this edge-case cannot happen at all.

And for the assembler programmers, we have documented the best practice to use MOVE 0xF000, SR (and we called it "best-practice" there and not obligation): So this is also OK for me.

Bottom Line:

I will email Volker that he is right and we can leave VBCC as it is.