[AVR] Mark call-clobbered registers as clobbered in interrupt handlers

[aykevl]

Summary

I have matched the RISCV backend, which only uses the interrupt save
list in getCalleeSavedRegs, _not_ in getCallPreservedMask. I don't know
the details of these two methods, but with it, the correct amount of
registers is saved and restored.

Without this patch, practically all interrupt handlers that call a
function will miscompile.

I have added a test to verify this behavior. I've also added a very
simple test to verify that more normal interrupt operations (in this
case, incrementing a global value) behave as expected.

[aykevl]

Forgot the colon in CHECK-NEXT

[aykevl]

Fixed missing semicolons in test

[benshi001]

Generally speaking, I am OK with this patch. Just one suggestion, run llvm/utils/update_llc_test_checks.py against the test case llvm/test/CodeGen/AVR/interrupts.ll

llvm/test/CodeGen/AVR/interrupts.ll

1

Would it be better to add an option -verify-machineinstrs ?

115

Why this is a CHECK not a CHECK-NEXT ? Would it be better to show all auto generated assembly ?

145

Why this is a CHECK not a CHECK-NEXT ? Would it be better to show all auto generated assembly ?

[aykevl]

• replaced CHECK: with CHECK-NEXT:

[aykevl]

In D116551#3221666, @benshi001 wrote:

Just one suggestion, run llvm/utils/update_llc_test_checks.py against the test case llvm/test/CodeGen/AVR/interrupts.ll

I think it is better to not do this for this test. With update_llc_test_checks.py, all the register names would be hidden. But the register names (r0, r1, etc) are an important part of this test.

llvm/test/CodeGen/AVR/interrupts.ll

1

Not sure. I've looked at other targets and most have it only on a small subset of tests.
If you want to enable the machine verifier, you can build LLVM with -DLLVM_ENABLE_EXPENSIVE_CHECKS=True or you can run llvm-lit manually, in my case:

../llvm-build.main/bin/llvm-lit -Dllc="llc -verify-machineinstrs" -sv llvm/test/CodeGen/AVR/

I usually use the above command to check for verification issues. This is how I found many issues that I fixed over several patches (see D107853#3224989).

[aykevl]

• rebased

[aykevl]

• rebase (try 2, previous one was the wrong patch)

[benshi001]

That gets conformed with avr-gcc's behaviour on handling interrupt/signal service functions.

For the following C code

#include <avr/interrupt.h>
void foo(void);
SIGNAL(_VECTOR(1)) { foo(); }

avr-gcc generates the following asm, which is conformed with your patch.

        .file   "a.c"
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__SREG__ = 0x3f
__tmp_reg__ = 0
__zero_reg__ = 1
        .text
.global __vector_1
        .type   __vector_1, @function
__vector_1:
        push r1
        push r0
        in r0,__SREG__
        push r0
        clr __zero_reg__
        push r18
        push r19
        push r20
        push r21
        push r22
        push r23
        push r24
        push r25
        push r26
        push r27
        push r30
        push r31
/* prologue: Signal */
/* frame size = 0 */
/* stack size = 15 */
.L__stack_usage = 15
        call foo
/* epilogue start */
        pop r31
        pop r30
        pop r27
        pop r26
        pop r25
        pop r24
        pop r23
        pop r22
        pop r21
        pop r20
        pop r19
        pop r18
        pop r0
        out __SREG__,r0
        pop r0
        pop r1
        reti

llvm/lib/Target/AVR/AVRCallingConv.td

<[
   CCAssignToStack<2, 1>
 ]>;
 
 //===----------------------------------------------------------------------===//
 // Callee-saved register lists.
 //===----------------------------------------------------------------------===//
 
 def CSR_Normal : CalleeSavedRegs<(add R29, R28, (sequence "R%u", 17, 2))>;
-def CSR_Interrupts : CalleeSavedRegs<(add (sequence "R%u", 31, 0))>;
+def CSR_Interrupts : CalleeSavedRegs<(add (sequence "R%u", 31, 2))>;

llvm/lib/Target/AVR/AVRRegisterInfo.cpp

AVRRegisterInfo::getCalleeSavedRegs(const MachineFunction *MF) const {
   return AFI->isInterruptOrSignalHandler()
               ? CSR_Interrupts_SaveList
               : CSR_Normal_SaveList;
 }
 
 const uint32_t *
 AVRRegisterInfo::getCallPreservedMask(const MachineFunction &MF,
                                       CallingConv::ID CC) const {
-  const AVRMachineFunctionInfo *AFI = MF.getInfo<AVRMachineFunctionInfo>();
-
-  return AFI->isInterruptOrSignalHandler()
-              ? CSR_Interrupts_RegMask
-              : CSR_Normal_RegMask;
+  return CSR_Normal_RegMask;
 }
 
 BitVector AVRRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
   BitVector Reserved(getNumRegs());
 
   // Reserve the intermediate result registers r1 and r2
   // The result of instructions like 'mul' is always stored here.
   Reserved.set(AVR::R0);

llvm/test/CodeGen/AVR/interrupts.ll

 ; RUN: llc < %s -march=avr | FileCheck %s
 
+@count = global i8 0
+
 define avr_intrcc void @interrupt_handler() {
 ; CHECK-LABEL: interrupt_handler:
 ; CHECK: sei
 ; CHECK-NEXT: push r0
 ; CHECK-NEXT: push r1
 ; CHECK-NEXT: in r0, 63
 ; CHECK-NEXT: push r0
 ; CHECK: clr r0

 ; CHECK-NEXT: out 63, r0
 ; CHECK-NEXT: pop r1
 ; CHECK-NEXT: pop r0
 ; CHECK-NEXT: reti
   alloca i8
   ret void
 }
 
+define void @signal_handler_with_increment() #1 {
+; CHECK-LABEL: signal_handler_with_increment:
+; CHECK:      push r0
+; CHECK-NEXT: push r1
+; CHECK-NEXT: in r0, 63
+; CHECK-NEXT: push r0
+; CHECK:      clr r0
+; CHECK-NEXT: clr r1
+; CHECK-NEXT: push r24
+; CHECK-NEXT: lds r24, count
+; CHECK-NEXT: inc r24
+; CHECK-NEXT: sts count, r24
+; CHECK-NEXT: pop r24
+; CHECK:      pop r0
+; CHECK-NEXT: out 63, r0
+; CHECK-NEXT: pop r1
+; CHECK-NEXT: pop r0
+; CHECK-NEXT: reti
+  %old = load volatile i8, i8* @count
+  %new = add i8 %old, 1
+  store volatile i8 %new, i8* @count
+  ret void
+}
+
+declare void @foo()
+
+; When a signal handler calls a function, it must push/pop all call clobbered
+; registers.
+define void @signal_handler_with_call() #1 {
+; CHECK-LABEL: signal_handler_with_call:
+; CHECK:      push r0
+; CHECK-NEXT: push r1
+; CHECK-NEXT: in r0, 63
+; CHECK-NEXT: push r0
+; CHECK:      clr r0
+; CHECK-NEXT: clr r1
+; CHECK:      push r18
+; CHECK-NEXT: push r19
+; CHECK-NEXT: push r20
+; CHECK-NEXT: push r21
+; CHECK-NEXT: push r22
+; CHECK-NEXT: push r23
+; CHECK-NEXT: push r24
+; CHECK-NEXT: push r25
+; CHECK-NEXT: push r26
+; CHECK-NEXT: push r27
+; CHECK-NEXT: push r30
+; CHECK-NEXT: push r31
+; CHECK-NEXT: call foo
+; CHECK-NEXT: pop r31
+; CHECK-NEXT: pop r30
+; CHECK-NEXT: pop r27
+; CHECK-NEXT: pop r26
+; CHECK-NEXT: pop r25
+; CHECK-NEXT: pop r24
+; CHECK-NEXT: pop r23
+; CHECK-NEXT: pop r22
+; CHECK-NEXT: pop r21
+; CHECK-NEXT: pop r20
+; CHECK-NEXT: pop r19
+; CHECK-NEXT: pop r18
+; CHECK:      pop r0
+; CHECK-NEXT: out 63, r0
+; CHECK-NEXT: pop r1
+; CHECK-NEXT: pop r0
+; CHECK-NEXT: reti
+  call void @foo()
+  ret void
+}
+
 attributes #0 = { "interrupt" }
 attributes #1 = { "signal" }