5
\$\begingroup\$

I'm trying to optimize my RX and TX interrupts to meet maximum execution time of 25 cycles while interrupts are disabled.

So far, I have found that code is optimized enough, but pushing and popping series of registers between loading and unloading __SREG__ exceeds the time limit.

 272:   80 91 24 01     lds r24, 0x0124
 276:   8f 5f           subi    r24, 0xFF   ; 255
 278:   8f 71           andi    r24, 0x1F   ; 31
 27a:   90 91 c6 00     lds r25, 0x00C6
 27e:   20 91 25 01     lds r18, 0x0125
 282:   28 17           cp  r18, r24
 284:   39 f0           breq    .+14        ; 0x294 <__vector_18+0x30>
 286:   e8 2f           mov r30, r24
 288:   f0 e0           ldi r31, 0x00   ; 0
 28a:   ea 5d           subi    r30, 0xDA   ; 218
 28c:   fe 4f           sbci    r31, 0xFE   ; 254
 28e:   90 83           st  Z, r25
 290:   80 93 24 01     sts 0x0124, r24

The only way to be able to place __SREG__ in safest location (include as much pushes as possible in unconscious area) was inline asm.

Here is my current code:

ISR(RX0_INTERRUPT, ISR_NAKED)
{
    //push
    asm volatile("push r31" ::); // table pointer
    asm volatile("push r30" ::); // table pointer
    asm volatile("push r25" ::); // received character
    asm volatile("push r18" ::); // once compared to r24 -> rx0_first_byte

    asm volatile("push r24" ::); // most stuff is executed in r24
    asm volatile("in r24,__SREG__" ::); // - 
    asm volatile("push r24" ::); // but one byte more on stack

    register uint8_t tmp_rx_last_byte = (rx0_last_byte + 1) & RX0_BUFFER_MASK;
    register uint8_t tmp = UDR0_REGISTER;

    if(rx0_first_byte != tmp_rx_last_byte)
    {
        rx0_buffer[tmp_rx_last_byte] = tmp;
        rx0_last_byte = tmp_rx_last_byte;
    }

    //pop
    asm volatile("pop r24" ::);
    asm volatile("out __SREG__,r24" ::);
    asm volatile("pop r24" ::);

    asm volatile("pop r18" ::);
    asm volatile("pop r25" ::);
    asm volatile("pop r30" ::);
    asm volatile("pop r31" ::);

    reti();
}

As you can see, there are a hardcoded register pushes that my compiler used, btw it's working at all, but I'm not sure how portable is it.

The only register which I can get by "=r" specificator is r24 and it's happening even for mask and rx0_first_byte.

So how can I tell the compiler to push/pop these 5 registers, even if they will be placed elsewhere?

What is possibility that the compiler will use r19 and r26 instead of r18 and r25?

I don't want to rewrite the whole ISR into assembler.

EDIT: thanks for all suggestions, finally I rewrote ISR into asm

    ISR(RX0_INTERRUPT, ISR_NAKED)
{
    asm volatile("\n\t"                      /* 5 ISR entry */
    "push  r31 \n\t"                         /* 2 */
    "push  r30 \n\t"                         /* 2 */
    "push  r25 \n\t"                         /* 2 */
    "push  r24 \n\t"                         /* 2 */
    "push  r18 \n\t"                         /* 2 */
    "in    r18, __SREG__ \n\t"               /* 1 */
    "push  r18 \n\t"                         /* 2 */

    /* read byte from UDR register */
    "lds   r25, %M[uart_data] \n\t"          /* 2 */

    /* load globals */
    "lds   r24, (rx0_last_byte) \n\t"        /* 2 */
    "lds   r18, (rx0_first_byte) \n\t"       /* 2 */

    /* add 1 & mask */
    "subi  r24, 0xFF \n\t" //???                  /* 1 */
    "andi  r24, %M[mask] \n\t"            /* 1 */

    /* if head == tail */
    "cp    r18, r24 \n\t"                    /* 1 */
    "breq  L_%= \n\t"                        /* 1/2 */

    "mov   r30, r24 \n\t"                    /* 1 */
    "ldi   r31, 0x00 \n\t"                   /* 1 */
    "subi  r30, lo8(-(rx0_buffer))\n\t"      /* 1 */
    "sbci  r31, hi8(-(rx0_buffer))\n\t"      /* 1 */
    "st    Z, r25 \n\t"                      /* 2 */
    "sts   (rx0_last_byte), r24 \n\t"        /* 2 */

"L_%=:\t"
    "pop   r18 \n\t"                         /* 2 */
    "out   __SREG__ , r18 \n\t"              /* 1 */
    "pop   r18 \n\t"                         /* 2 */
    "pop   r24 \n\t"                         /* 2 */
    "pop   r25 \n\t"                         /* 2 */
    "pop   r30 \n\t"                         /* 2 */
    "pop   r31 \n\t"                         /* 2 */
    "reti \n\t"                              /* 5 ISR return */

    : /* output operands */

    : /* input operands */
    [uart_data] "M"    (_SFR_MEM_ADDR(UDR0_REGISTER)),
    [mask]      "M"    (RX0_BUFFER_MASK)

    /* no clobbers */
    );

}

UPDATE:

After some tests I have found that the interrupts are disabled before entering ISR handler, not after unloading __SREG__ as I have been suggested before.

The only way is to globalize registers as ndim suggested, or use the following code:

ISR(RX0_INTERRUPT, ISR_NAKED)
    {
        asm volatile("\n\t"                      /* 4 ISR entry */

        "push  r0 \n\t"                          /* 2 */
        "in    r0, __SREG__ \n\t"                /* 1 */

        "push  r31 \n\t"                         /* 2 */
        "push  r30 \n\t"                         /* 2 */
        "push  r25 \n\t"                         /* 2 */
        "push  r24 \n\t"                         /* 2 */
        "push  r18 \n\t"                         /* 2 */

        /* read byte from UDR register */
        "lds   r25, %M[uart_data] \n\t"          /* 2 */

#ifdef USART_UNSAFE_RX_INTERRUPT // enable interrupt after satisfying UDR register
        "sei \n\t"                               /* 1 */
#endif
        /* load globals */
        "lds   r24, (rx0_last_byte) \n\t"        /* 2 */
        "lds   r18, (rx0_first_byte) \n\t"       /* 2 */

        /* tmp_rx_last_byte = (rx0_last_byte + 1) & RX0_BUFFER_MASK */
        "subi  r24, 0xFF \n\t"                   /* 1 */
        "andi  r24, %M[mask] \n\t"               /* 1 */

        /* if(rx0_first_byte != tmp_rx_last_byte) */
        "cp    r18, r24 \n\t"                    /* 1 */
        "breq  .+14 \n\t"                        /* 1/2 */

        /* rx0_buffer[tmp_rx_last_byte] = tmp */
        "mov   r30, r24 \n\t"                    /* 1 */
        "ldi   r31, 0x00 \n\t"                   /* 1 */
        "subi  r30, lo8(-(rx0_buffer))\n\t"      /* 1 */
        "sbci  r31, hi8(-(rx0_buffer))\n\t"      /* 1 */
        "st    Z, r25 \n\t"                      /* 2 */

        /* rx0_last_byte = tmp_rx_last_byte */
        "sts   (rx0_last_byte), r24 \n\t"        /* 2 */

#ifdef USART_UNSAFE_RX_INTERRUPT
        "cli \n\t"                               /* 1 */
#endif

        "pop   r18 \n\t"                         /* 2 */
        "pop   r24 \n\t"                         /* 2 */
        "pop   r25 \n\t"                         /* 2 */
        "pop   r30 \n\t"                         /* 2 */
        "pop   r31 \n\t"                         /* 2 */

        "out   __SREG__ , r0 \n\t"               /* 1 */
        "pop   r0 \n\t"                          /* 2 */

        "reti \n\t"                              /* 4 ISR return */

        : /* output operands */

        : /* input operands */
        [uart_data] "M"    (_SFR_MEM_ADDR(UDR0_REGISTER)),
        [mask]      "M"    (RX0_BUFFER_MASK)

        /* no clobbers */
        );

    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Would it suit your overall program to simply set a flag in the ISR and do the processing outside the ISR? \$\endgroup\$ – David Aug 31 '15 at 12:32
  • 2
    \$\begingroup\$ Do you realize that next time you compiler your compiler might generate different code, using different registers? \$\endgroup\$ – Wouter van Ooijen Aug 31 '15 at 12:47
  • 2
    \$\begingroup\$ I'm not working on AVR, but as a general rule of thumb: never rely on random placements of the compiler, a small version change might alter the behaviour, a small code change might as well. You might also read your compiler manual very carefully, there are real gems in there for critical stuff (pragmas, intrinsics) \$\endgroup\$ – Arsenal Aug 31 '15 at 12:48
  • 2
    \$\begingroup\$ I don't think you should do the pushes manually for a piece of compiler-generated code. You must know which registers are used, which you in general can't know. Unless of course you push all that are required to be saved by the calling convention. \$\endgroup\$ – Wouter van Ooijen Aug 31 '15 at 13:19
  • 1
    \$\begingroup\$ If it's only 25 cycles long, what's wrong with writing the whole thing in assembly code? Then you don't need to follow the compiler conventions at all -- just save the registers you use inside the ISR and restore them at the end. You'll probably be able to achieve even greater overall optimization. \$\endgroup\$ – Dave Tweed Aug 31 '15 at 14:05
1
\$\begingroup\$

Saving a few pushes/pops by using global register variables, putting all instructions into one asm() statement, I would arrive at something like

#define RB_WIDTH 5
#define RB_SIZE (1<<(RB_WIDTH))
#define RB_MASK ((RB_SIZE)-1)

register uint8_t rb_head      asm("r13");
register uint8_t rb_tail      asm("r14");
register uint8_t rb_sreg_save asm("r15");

volatile uint8_t rb_buf[RB_SIZE];

ISR(USART0_RX_vect, ISR_NAKED)                 /* CLOCK CYCLES */
{
  asm("\n\t"                                   /* 5 ISR entry */
      "push  r24\n\t"                          /* 2 */
      "push  r25\n\t"                          /* 2 */
      "push  r30\n\t"                          /* 2 */
      "push  r31\n\t"                          /* 2 */
      "in    %r[sreg_save], __SREG__\n\t"      /* 1 */
      "\n\t"

      /* read byte from UART */
      "lds   r25, %M[uart_data]\n\t"           /* 2 */

      /* next_tail := (cur_tail + 1) & MASK; */
      "ldi   r24, 1\n\t"                       /* 1 */
      "add   r24, %r[tail]\n\t"                /* 1 */
      "andi  r24, %a[mask]\n\t"                /* 1 */

      /* if next_tail == cur_head */
      "cp    r24, %r[head]\n\t"                /* 1 */
      "breq  L_%=\n\t"                         /* 1/2 */

      /* rb_buf[next_tail] := byte */
      "mov   r30, r24\n\t"                     /* 1 */
      "ldi   r31, 0\n\t"                       /* 1 */
      "subi  r30, lo8(-(rb_buf))\n\t"          /* 1 */
      "sbci  r31, hi8(-(rb_buf))\n\t"          /* 1 */
      "st    Z, r25\n\t"                       /* 2 */

      /* rb_tail := next_tail */
      "mov   %r[tail], r24\n\t"                /* 1 */

      "\n"
"L_%=:\t"
      "out   __SREG__, %r[sreg_save]\n\t"      /* 1 */
      "pop   r31\n\t"                          /* 2 */
      "pop   r30\n\t"                          /* 2 */
      "pop   r25\n\t"                          /* 2 */
      "pop   r24\n\t"                          /* 2 */
      "reti\n\t"                               /* 5 ISR return */
      : /* output operands */
        [tail]      "+r"   (rb_tail)    /* both input+output */
      : /* input operands */
        [uart_data] "M"    (_SFR_MEM_ADDR(UDR0)),
        [mask]      "M"    (RB_MASK),
        [head]      "r"    (rb_head),
        [sreg_save] "r"    (rb_sreg_save)
        /* no clobbers */
      );
}

Reorganizing the ring buffer code a bit might reduce the amount of code in the ISR writing to the ring buffer even simpler (at the cost of making the function reading from the buffer more complex).

I have put up a complete example with build system and support structures at https://github.com/ndim/avr-uart-example/

\$\endgroup\$
  • \$\begingroup\$ Looks like I totally misunderstood all inline asm guides especially operands. I'll try to write it without aligning global variables into registers ;) \$\endgroup\$ – jnk0le Sep 5 '15 at 21:53
  • \$\begingroup\$ It looks like gcc treats uninitialized variables as a r24. \$\endgroup\$ – jnk0le Sep 5 '15 at 23:10
  • \$\begingroup\$ So as I can see, best way is to just write all code in assembler. \$\endgroup\$ – jnk0le Sep 6 '15 at 12:13
1
\$\begingroup\$

A few helpful things I found while making an AVR ISR short and fast for https://github.com/ndim/freemcan/tree/master/firmware were:

  • Have your build system generate assembly language dumps of your generated code during every rebuild and observe the changes in the generated code every time you change the source. It really helps to see what actually happens. (I use avr-objdump -h -S firmware.elf > firmware.lss.)

  • If you need a really fast ISR, you might save some cycles for pushing/popping registers by telling avr-gcc to compile all C code without using some registers (e.g. -ffixed-r13), and then use those registers als global variables in the ISR without pushing/popping. This also saves you the extra cycles for memory access. The head and tail pointers for the ring buffer are candidates in your case.

  • I can't remember off-hand whether the avr-gcc generated ISR always pushes/pops all registers, or just the ones it actually uses. If it pushes/pops more than absolutely needed, you might have to write the ISR in assembly after all.

  • You can then still take the generated assembly language instructions, put them into a .S assembly source file and hand-optimize that even more.

In my use case, however, it turned out that the ISR wasn't that time critical after all.

BTW, I would use the inline asm parameters to let gcc select the registers instead of hardcoding them. See http://www.nongnu.org/avr-libc/user-manual/inline_asm.html

\$\endgroup\$
  • \$\begingroup\$ GCC adds code for pushing r0 and r1 (+ zeroing it) and does disabling interrupts after first push, meanwhile I want to do pushes first and then disable interrupts to fit into 25 cycleswhile interrups are disabled. I thought about rewrinting ISR into assembler but I need lot of avr-asm basics. Inline asm is even worser than normal one -how to get to global variables, and how to pass used registers by inline asm parametrs (I'm able to get r24 and this manual is not enough helpfull) \$\endgroup\$ – jnk0le Sep 1 '15 at 10:42
  • \$\begingroup\$ Do the examples in github.com/ndim/freemcan/blob/master/firmware/table-element.h help with how to get to variables from inline asm? \$\endgroup\$ – ndim Sep 3 '15 at 20:51
  • \$\begingroup\$ Well, if you want to have different ISR preamble and cleanup code from the one avr-gcc generates, you either have to change avr-gcc or (much easier), write assembly yourself. Use the avr-gcc generated assembly code as the foundation, and go from there. \$\endgroup\$ – ndim Sep 3 '15 at 20:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.