-
Notifications
You must be signed in to change notification settings - Fork 12k
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
[AVR] Support return address intrinsics #67266
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
710fcf3
to
93331c4
Compare
Tagging @benshi001 as AVR backend code owner |
; CHECK-NEXT: in r29, 62 | ||
; CHECK-NEXT: mov r24, r28 | ||
; CHECK-NEXT: mov r25, r29 | ||
; CHECK-NEXT: adiw r24, 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not always true.
- The PC register has 2-byte length on some earlier AVR devices.
- The port 61 & 62 are not always available on some earlier AVR devices.
You need to distinguish the result on different devices, such as
RUN: llc < %s -mtriple=avr -mcpu=avr2 | FileCheck --check-prefix=AVR2 %s
RUN: llc < %s -mtriple=avr -mcpu=avr6 | FileCheck --check-prefix=AVR6 %s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is the same for all -mcpu
values. There already is expand<AVR::SPREAD>
which outside of the PR doesn't have a check for some kind of "SP available". This PR does expose it more, so I guess I can include this check. Do I have to check datasheets for each of the supported targets if it has SPH SPL? Or just each AVR family?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this affects old chips specifically if they had 256B or less. Which we have no way of checking. What GCC seems to do is let the user set -mtiny-stack
and also maybe have per-architecture defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC accepts two options: SP as implemented here, or short SP (only SPL). Looks like it can be degraded to short SP with -mtiny-stack
. Full list of AVRs with short SPs as listed by GCC:
at90s2313
at90s2323
at90s2333
at90s2343
attiny22
attiny26
at90s4433
attiny13
attiny13a
attiny2313
attiny2313a
attiny24
attiny24a
attiny25
attiny261
attiny261a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we have an equivalent small stack subtarget feature, so I used it as a condition in SPREAD and SPWRITE lowering
93331c4
to
24f98d3
Compare
24f98d3
to
4215091
Compare
I suggest you split your patch to two ones. The first one fixes the wrong operation SPREAD/SPWRITE on tiny-stack devices, at least this part I think it is good. Regarding the second part, I think there are still some spots, especially you need to distinguish devices with 2-byte PC and 3-byte PC, which does affect the address of return address on the stack. |
This PR allows the use of @llvm.returnaddress and @llvm.addressofreturnaddress. Zig's runtime checks currently use these for some sort of unwinding mechanism. All code used in this implementation has been copied from PPC/MSP430/M68k/X86 and relies on functions exposed by generic LLVM code.