[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
https://github.com/carlos4242 closed https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: That makes sense. I think I’ll close this PR for now and work on the apple fork to get these issues resolved with them. Then I’ll come back once we have squared off the edges. Cheers Ben. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
benshi001 wrote: > Sure thing @benshi001 ... I'll create tests. The Swift ABI is documented > here: https://github.com/apple/swift/blob/main/docs/ABI/CallingConvention.rst > > I don't think this change will actually change any ABI. All my code uses the > normal avr-gcc ABI you referenced. > > I'll work out the details with the Apple engineers on a PR in their LLVM > fork. Once we've got it in a state everyone is happy with, I'll bring it back > here. That way nothing is blocked and we can take the time we need. Thank you. It would be better to mention this ABI document link in the source code. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: Sure thing @benshi001 ... I'll create tests. The Swift ABI is documented here: https://github.com/apple/swift/blob/main/docs/ABI/CallingConvention.rst I don't think this change will actually change any ABI. All my code uses the normal avr-gcc ABI you referenced. I'll work out the details with the Apple engineers on a PR in their LLVM fork. Once we've got it in a state everyone is happy with, I'll bring it back here. That way nothing is blocked and we can take the time we need. Thank you. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
benshi001 wrote: I am not familiar with swift. So is there an official document for the SWIFT specific ABI on AVR? I think this is necessary, just like C++: https://gcc.gnu.org/wiki/avr-gcc. And what's more, tests are required to be committed with functional changes, this is a requirement for all llvm commits with few exception. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
rjmccall wrote: > That makes a lot of sense. Thank you John. I guess here are my thoughts. As I > understand it, the `SwiftABIInfo` by default does something like "if > something can be passed in 4 registers or fewer then pass by register, > otherwise pass indirectly"? I think that sweet spot (also sort of reflected > in Existential Containers I believe?) makes sense for 32 bit or 64 bit > registers (and presumably the sorts of caching you'd expect in modern > intel/arm larger machine architectures). We have 8 bit registers, which is > quite different... Yes, I see. Pointers are 16-bit and would be passed in two registers, right? You need to do more accurate counting than the default implementation. The default implementation breaks up "large" integers (also specified by the ABI) when determining the scalar sequence, and then it assumes that all scalars count the same towards the limit. You probably want to keep e.g. `i16`s and `ptr`s in the scalar sequence instead of splitting them into `i8`s, but you want to count them properly as two registers rather than one. And then, yeah, maybe it makes sense to put the cap at something like 6 or even 4 registers. > In our case, one complication is stack manipulation is fairly painful, we may > be able to improve the AVR back end a bit but at the moment, just moving the > stack pointer down for something like an alloca is 8 instructions (and then > again 8 bytes in the function epilog). On most architectures, functions that include calls will need to perform a stack adjustment on entry anyway — at the very least, for the function's own frame — but you don't need an extra adjustment for individual call sites because you include space for the maximum stack argument space usage of all the calls in the function in that initial adjustment. Is that not how it's done on AVR? > But, equally, the default C ABI (avr gcc abi) allows a struct to be split > over as many as 18 registers, and we are often producing pretty inefficient > code like this when we have large structs, moving registers around a lot > either side of a call site. Which is something I really wanted to find a way > to solve "one day" with Swift for Arduino/Microswift/Swift for AVR. Yeah, 18 registers seems like it'd be way higher than you want. I usually approach this from a code-size optimization perspective. Passing in registers is great in the ideal situation: the caller is able to efficiently produce the argument into whatever argument register is convenient (e.g. loading, or loading an immediate, or taking the address of a local variable), and the callee is going to immediately use the component values of argument in whatever register it came in. When that *isn't* true, you don't want passing in registers to be a highly punitive mistake. That's why the cap is normally expressed in terms of the number of scalars involved rather than the size of the data: the assumption is that each scalar will require a separate load/store if we're not in the ideal case. And for ISAs like AVR with non-orthogonal register use, I feel like the chances that taking complex data in registers will be useful to the callee are somewhat lower; and if spilling an argument to the stack retroactively is expensive, that also changes the balance. So having a relatively low cap seems like the right thing to do. > Probably it's reasonable to say that ideally, when lowering to AVR assembly > from Swift, any struct larger than 8 bytes should be passed on the stack in > our case. Do you think we can implement that? It's important to distinguish the three ways of passing an argument: you can pass it in registers, you can pass it in the stack argument area, and you can pass a pointer to it. I am generally of the opinion that passing arguments in the stack argument area is a waste and should only be used if you don't have a better choice available, e.g. because you have too many arguments and you've run out of registers. This is because you usually can't forward such an argument efficiently: you have to copy it into a *new* stack argument area for the next call. So the Swift CC usually says that if an argument is too big for the cap, it gets passed by pointer. So I guess it depends on what you mean by "passed on the stack". I would not recommend passing in the stack argument area. But triggering pass-by-pointer on relatively small structures makes sense to me. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
benshi001 wrote: Please refer to my previous commit of AVR ABI, how tests are provided for a functional change. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
benshi001 wrote: As I have suggested, any functional change need tests. So I think you need to add some tests to show what your changes affect. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: @benshi001 Have you got any thoughts on this as the AVR maintainer? I've been using various versions of this patch in my own branches for years. Should we merge? I think ultimately it's your call as you're the AVR backend owner? https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: > This is less about the implementation weeds of LLVM and more about the actual > details of your calling convention — the decisions about how arguments and > results are passed that are ultimately downstream of the choices made here. > Mostly, I'm encouraging you as a platform maintainer who's adding Swift CC > support for your platform to take a few moments to make sure you're happy > with the CC. It's likely that everything will _work_ regardless — Swift will > generate both declarations and calls according to the rules of the CC you > specify here, which should be enough to make things functional — but you > might find yourself looking at generated code in two years and realize you've > been doing something ridiculous for that entire time, and maybe it'll be too > awkward to change. That makes a lot of sense. Thank you John. I guess here are my thoughts. As I understand it, the `SwiftABIInfo` by default does something like "if something can be passed in 4 registers or fewer then pass by register, otherwise pass indirectly"? I think that sweet spot (also sort of reflected in Existential Containers I believe?) makes sense for 32 bit or 64 bit registers (and presumably the sorts of caching you'd expect in modern intel/arm larger machine architectures). We have 8 bit registers, which is quite different... Thinking aloud... In our case, one complication is stack manipulation is fairly painful, we may be able to improve the AVR back end a bit but at the moment, just moving the stack pointer down for something like an alloca is 8 instructions (and then again 8 bytes in the function epilog). But, equally, the default C ABI (avr gcc abi) allows a struct to be split over as many as 18 registers, and we are often producing pretty inefficient code like this when we have large structs, moving registers around a lot either side of a call site. Which is something I really wanted to find a way to solve "one day" with Swift for Arduino/Microswift/Swift for AVR. Probably it's reasonable to say that ideally, when lowering to AVR assembly from Swift, any struct larger than 8 bytes should be passed on the stack in our case. Do you think we can implement that? I'm happy to do some AVR back end work to change the ABI when the swift calling convention is in place on a function. (Although it might have to wait until I've had time to get the new Embedded mode working on our Swift compiler first, as that's taking up most of my time for the moment.) https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
rjmccall wrote: This is less about the implementation weeds of LLVM and more about the actual details of your calling convention — the decisions about how arguments and results are passed that are ultimately downstream of the choices made here. Mostly, I'm encouraging you as a platform maintainer who's adding Swift CC support for your platform to take a few moments to make sure you're happy with the CC. It's likely that everything will *work* regardless — Swift will generate both declarations and calls according to the rules of the CC you specify here, which should be enough to make things functional — but you might find yourself looking at generated code in two years and realize you've been doing something ridiculous for that entire time, and maybe it'll be too awkward to change. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: Thanks, that sounds like it's worth looking into and might avoid issues with AVR. I'm still nowhere near enough of an LLVM expert to follow all the aspects of the discussion. Although from our perspective, I've never seen an issue that I know, using the above patch for the last few years in our LLVM back end for the swift compile (not in our build of clang). The code produced by the back end has always followed the AVR GCC ABI calling convention (https://gcc.gnu.org/wiki/avr-gcc ...roughly... registers assigned to parameters starting at r24 and working backward r22, r20, etc. to r8 then the rest on stack) when I have examined the generated assembly. But maybe that is because, until now, only our swift code has been emitted with the swift calling convention, until #71986 is merged, clang can't emit code with that calling convention and that's the issue? Stupid question... Is it possible that our AVR LLVM back end is basically ignoring the swift calling convention when it actually emits our code? Doesn't usually the front end (clang/swift) just specify parameter types and the back end decides whether to put something in registers or on the stack? Sorry if I'm being dumb! --- It might be that I just don't understand enough about LLVM and clang to really understand what all the surrounding code is doing properly, and maybe the right answer is for me to close this PR and wait for someone more experienced to come along when the time is right. We can always maintain this as a local patch in our builds instead, as we have for the past few years? I don't want to lay down issues for anyone using this in the future. Or as an alternative, we can merge this PR as-is and deal with AVR problems later as an optimisation. I am neutral about what is the best approach and happy to take the advice of all the experts on here. (At the very least, the discussion has been enlightening for me!) https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
rjmccall wrote: > @efriedma-quic Cool. So it sounds like it's worth parking this for now, until > Kuba's work #71986 is merged? > > @rjmccall I'm not 100% sure I understand? The existing code in AVR.cpp > handles the standard AVR ABI, which has a few simple rules based on GCC > behaviour. Here we are effectively just passing on the handling for that to > `AVRABIInfo`. The error in register thing, I just copied what ARM is doing. I > figure that's OK for now. If we need to do some bugfixes later to get AVR > working with errors then we'll take a look then probably. Is that reasonable? `SwiftABIInfo` exists so that targets can tweak more details than just whether to use `swifterror`. For example, you can change the conditions under which we'll try to return things in registers, and if you don't do that, IIRC Swift will try to return quite a few things in registers by default. I'm just saying that you should make sure that that matches with your backend — either your backend needs to make sure that it returns a type like `{ ptr, ptr, float, float }` in registers for `swiftcc` or you should tweak the frontend so that it knows to return it directly. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
carlos4242 wrote: @efriedma-quic Cool. So it sounds like it's worth parking this for now, until Kuba's work #71986 is merged? @rjmccall I'm not 100% sure I understand? The existing code in AVR.cpp handles the standard AVR ABI, which has a few simple rules based on GCC behaviour. Here we are effectively just passing on the handling for that to `AVRABIInfo`. The error in register thing, I just copied what ARM is doing. I figure that's OK for now. If we need to do some bugfixes later to get AVR working with errors then we'll take a look then probably. Is that reasonable? https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
rjmccall wrote: Right, that would be the way to test it. I don't know much about AVR, but you should also look at some of the parameters to the lowering (e.g. how many max values it's okay to break an aggregate into) and make sure you're happy with them. https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
efriedma-quic wrote: See also #71986 https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
efriedma-quic wrote: Can you not test this by checking an `__attribute__((swiftcall))` function works in C? https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
https://github.com/carlos4242 updated https://github.com/llvm/llvm-project/pull/72298 >From 333916a07e90955564d03f14e004695802d9f618 Mon Sep 17 00:00:00 2001 From: Carl Peto Date: Tue, 14 Nov 2023 17:27:37 + Subject: [PATCH] [AVR] make the AVR ABI Swift compatible This patch is needed to add support to clang's AVR ABI for the Swift language. It is a pre-requisite for upstreaming AVR patches to the Swift compiler itself. @benshi001 @rjmccall --- clang/lib/CodeGen/Targets/AVR.cpp | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clang/lib/CodeGen/Targets/AVR.cpp b/clang/lib/CodeGen/Targets/AVR.cpp index 50547dd6dec5e7d..ae141d3f485139c 100644 --- a/clang/lib/CodeGen/Targets/AVR.cpp +++ b/clang/lib/CodeGen/Targets/AVR.cpp @@ -112,7 +112,10 @@ class AVRABIInfo : public DefaultABIInfo { class AVRTargetCodeGenInfo : public TargetCodeGenInfo { public: AVRTargetCodeGenInfo(CodeGenTypes , unsigned NPR, unsigned NRR) - : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) {} + : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) { +SwiftInfo = +std::make_unique(CGT, /*SwiftErrorInRegister=*/true); + } LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const override { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
https://github.com/carlos4242 updated https://github.com/llvm/llvm-project/pull/72298 >From fe214850572ba740c0027e8f2908cde0bae75517 Mon Sep 17 00:00:00 2001 From: Carl Peto Date: Tue, 14 Nov 2023 17:27:37 + Subject: [PATCH] [AVR] make the AVR ABI Swift compatible This patch is needed to add support to clang's AVR ABI for the Swift language. It is a pre-requisite for upstreaming AVR patches to the Swift compiler itself. @benshi001 @rjmccall --- clang/lib/CodeGen/Targets/AVR.cpp | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clang/lib/CodeGen/Targets/AVR.cpp b/clang/lib/CodeGen/Targets/AVR.cpp index 50547dd6dec5e7d..bc6418c1e224eb4 100644 --- a/clang/lib/CodeGen/Targets/AVR.cpp +++ b/clang/lib/CodeGen/Targets/AVR.cpp @@ -112,7 +112,10 @@ class AVRABIInfo : public DefaultABIInfo { class AVRTargetCodeGenInfo : public TargetCodeGenInfo { public: AVRTargetCodeGenInfo(CodeGenTypes , unsigned NPR, unsigned NRR) - : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) {} + : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) { +SwiftInfo = + std::make_unique(CGT, /*SwiftErrorInRegister=*/true); + } LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const override { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
github-actions[bot] wrote: :warning: C/C++ code formatter, clang-format found issues in your code. :warning: You can test this locally with the following command: ``bash git-clang-format --diff 410f130bb99b88f1a8f21659d98053e6f3e5e8f6 ed8b63c31f8b9a496bc5c51ab83a132c224594f0 -- clang/lib/CodeGen/Targets/AVR.cpp `` View the diff from clang-format here. ``diff diff --git a/clang/lib/CodeGen/Targets/AVR.cpp b/clang/lib/CodeGen/Targets/AVR.cpp index bc6418c1e2..ae141d3f48 100644 --- a/clang/lib/CodeGen/Targets/AVR.cpp +++ b/clang/lib/CodeGen/Targets/AVR.cpp @@ -113,9 +113,9 @@ class AVRTargetCodeGenInfo : public TargetCodeGenInfo { public: AVRTargetCodeGenInfo(CodeGenTypes , unsigned NPR, unsigned NRR) : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) { -SwiftInfo = - std::make_unique(CGT, /*SwiftErrorInRegister=*/true); - } +SwiftInfo = +std::make_unique(CGT, /*SwiftErrorInRegister=*/true); + } LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const override { `` https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
llvmbot wrote: @llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Carl Peto (carlos4242) Changes This patch is needed to add support to clang's AVR ABI for the Swift language. It is a pre-requisite for adding AVR support to the public Swift compiler itself. I'm open to any suggestions how I might create suitable unit tests for this? @benshi001 @rjmccall --- Full diff: https://github.com/llvm/llvm-project/pull/72298.diff 1 Files Affected: - (modified) clang/lib/CodeGen/Targets/AVR.cpp (+4-1) ``diff diff --git a/clang/lib/CodeGen/Targets/AVR.cpp b/clang/lib/CodeGen/Targets/AVR.cpp index 50547dd6dec5e7d..bc6418c1e224eb4 100644 --- a/clang/lib/CodeGen/Targets/AVR.cpp +++ b/clang/lib/CodeGen/Targets/AVR.cpp @@ -112,7 +112,10 @@ class AVRABIInfo : public DefaultABIInfo { class AVRTargetCodeGenInfo : public TargetCodeGenInfo { public: AVRTargetCodeGenInfo(CodeGenTypes , unsigned NPR, unsigned NRR) - : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) {} + : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) { +SwiftInfo = + std::make_unique(CGT, /*SwiftErrorInRegister=*/true); + } LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const override { `` https://github.com/llvm/llvm-project/pull/72298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [AVR] make the AVR ABI Swift compatible (PR #72298)
https://github.com/carlos4242 created https://github.com/llvm/llvm-project/pull/72298 This patch is needed to add support to clang's AVR ABI for the Swift language. It is a pre-requisite for adding AVR support to the public Swift compiler itself. I'm open to any suggestions how I might create suitable unit tests for this? @benshi001 @rjmccall >From ed8b63c31f8b9a496bc5c51ab83a132c224594f0 Mon Sep 17 00:00:00 2001 From: Carl Peto Date: Tue, 14 Nov 2023 17:27:37 + Subject: [PATCH] [AVR] make the AVR ABI Swift compatible This patch is needed to add support to clang's AVR ABI for the Swift language. It is a pre-requisite for upstreaming AVR patches to the Swift compiler itself. @benshi001 @rjmccall --- clang/lib/CodeGen/Targets/AVR.cpp | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clang/lib/CodeGen/Targets/AVR.cpp b/clang/lib/CodeGen/Targets/AVR.cpp index 50547dd6dec5e7d..bc6418c1e224eb4 100644 --- a/clang/lib/CodeGen/Targets/AVR.cpp +++ b/clang/lib/CodeGen/Targets/AVR.cpp @@ -112,7 +112,10 @@ class AVRABIInfo : public DefaultABIInfo { class AVRTargetCodeGenInfo : public TargetCodeGenInfo { public: AVRTargetCodeGenInfo(CodeGenTypes , unsigned NPR, unsigned NRR) - : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) {} + : TargetCodeGenInfo(std::make_unique(CGT, NPR, NRR)) { +SwiftInfo = + std::make_unique(CGT, /*SwiftErrorInRegister=*/true); + } LangAS getGlobalVarAddressSpace(CodeGenModule , const VarDecl *D) const override { ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits