Skip to content
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

Replace std.builtin.CallingConvention with a tagged union, eliminating @setAlignStack #21697

Merged
merged 28 commits into from
Oct 23, 2024

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Oct 14, 2024

Huge thanks to @alexrp for the help with this branch -- I absolutely wouldn't have gotten through this without his help.


This PR implements #21209 by replacing std.builtin.CallingConvention with a tagged union and removing @setAlignStack.

The tags of CallingConvention enumerate all major calling conventions for all targets recognised by std.Target. A key design philosophy here is that a CallingConvention tag specifies a calling convention independently of details like the current target or compiler backend; so, rather than e.g. sysv, we have x86_64_sysv, x86_sysv, etc. There is no longer a single tag for the default C calling convention -- more on this in a bit.

The payload of a CallingConvention is a set of options which allow that calling convention to be customized. The vast majority of calling conventions right now have a CommonOptions payload, which allows overriding the incoming stack alignment of the function; that is, the expected boundary the stack pointer is aligned to when the function is called. A few calling conventions have other options; for instance, the x86_sysv tag uses X86RegparmOptions, which allows specifying register_params: u2, which provides an equivalent to Clang's __attribute__((regparm(x))).

A "raw" callconv annotation now has a quite verbose form; for instance, callconv(.{ .x86_64_sysv = .{} }). In most cases, rather than specifying the tag directly like this, the calling convention should instead be specified using one of a handful of declarations on CallingConvention, which are easily used via decl literals, giving a syntax similar to before:

callconv(.c)
// equivalent to
callconv(std.builtin.CallingConvention.c)
// which dispatches to something like...
callconv(.{ .x86_64_sysv = .{} })
// ...depending on target

Right now, there are declarations for c, winapi, and kernel. In addition, there are deprecated declarations acting as aliases for the old CallingConvention tags (Unspecified, C, Interrupt, etc), allowing this whole change to be effectively backwards-compatible in Zig 0.14.0 (note that these aliases should be removed in the 0.15.0 release cycle). In the future, we may want to add some more declarations for convenience; I chose to start small, since it's better to add these in future than to start with too many and break everyone's code when we delete them!


Follow-up tasks:

@xdBronch
Copy link
Contributor

zig/lib/std/zig/render.zig

Lines 188 to 189 in 7b8fc18

if (mem.eql(u8, "Inline", tree.tokenSlice(main_tokens[callconv_expr]))) {
try ais.writer().writeAll("inline ");

should this be updated now?

lib/std/Target.zig Outdated Show resolved Hide resolved
lib/std/builtin.zig Outdated Show resolved Hide resolved
lib/std/builtin.zig Outdated Show resolved Hide resolved
src/codegen/c.zig Outdated Show resolved Hide resolved
src/link/Dwarf.zig Outdated Show resolved Hide resolved
src/translate_c.zig Outdated Show resolved Hide resolved
src/codegen/llvm.zig Outdated Show resolved Hide resolved
src/codegen/llvm.zig Outdated Show resolved Hide resolved
src/codegen/llvm.zig Outdated Show resolved Hide resolved
src/codegen/llvm.zig Outdated Show resolved Hide resolved
@alexrp
Copy link
Member

alexrp commented Oct 14, 2024

This looks to be missing something like alexrp@77a7268. But note that this commit is incomplete: It's not lowering the interrupt argument, if any, and it also needs to set signal (instead of interrupt) on avr_signal functions. (Clang also sets noinline on interrupt handlers for some targets... but I don't see the point at all since you're not supposed to be able to call these normally in code anyway.)

lib/std/builtin.zig Outdated Show resolved Hide resolved
src/InternPool.zig Outdated Show resolved Hide resolved
src/Value.zig Outdated Show resolved Hide resolved
@mlugg mlugg mentioned this pull request Oct 15, 2024
lib/std/builtin.zig Outdated Show resolved Hide resolved
src/Sema.zig Outdated Show resolved Hide resolved
lib/std/builtin.zig Outdated Show resolved Hide resolved
lib/std/builtin.zig Outdated Show resolved Hide resolved
@mlugg mlugg force-pushed the callconv branch 2 times, most recently from 183570c to fec5634 Compare October 16, 2024 16:56
@mlugg mlugg force-pushed the callconv branch 2 times, most recently from 03d42dd to b77fe59 Compare October 17, 2024 10:26
…ode`

The RISC-V specification uses these terms a little interchangably, but
"mode" seems more correct here.
There are several more that we could support here, but I didn't feel
like going down the rabbit-hole of figuring them out. In particular,
some of the Clang enum fields aren't specific enough for us, so we'll
have to switch on the target to figure out how to translate-c them. That
can be a future enhancement.
These only worked before because our lowering of the `AAPCS` calling
convention was incorrect in a way which happened to match the ABI of
these functions. The tests aren't actually very helpful -- there are
already tests for `divmoddi4` etc -- so rather than using inline asm on
the caller side to match the ABI, we just delete these two tests.

We were for some reason missing a direct test for `__udivmodsi4`, so one
has been added.
The whole motivation behind this proposal in the first place was that
the LLVM backend disagrees with the self-hosted backends on what
`@setAlignStack` meant, so we can't just translate the old logic to the
new system! These backends can introduce support for overriding
`incoming_stack_alignment` later on.
Looks like the self-hosted riscv64 backend can't handle `std.meta.eql`
involving the new `CallingConvention` right now.
@mlugg mlugg added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. labels Oct 20, 2024
@mlugg mlugg requested a review from andrewrk October 20, 2024 00:19
@mlugg mlugg merged commit 6bf52b0 into ziglang:master Oct 23, 2024
10 checks passed
@mlugg
Copy link
Member Author

mlugg commented Oct 23, 2024

@alexrp the follow-up tasks listed in the PR are all yours now. Are you going to work on them imminently, or should I put up issues to track them?

@Rexicon226
Copy link
Contributor

@mlugg mind opening issues for the riscv items, I would like to handle them.

@alexrp
Copy link
Member

alexrp commented Oct 24, 2024

The follow-up work is near the top of my to-do list, so I'll get to it soon-ish. I'm ok letting @Rexicon226 take the RISC-V specific stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants