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

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

mlugg
Copy link
Member

@mlugg mlugg commented Oct 14, 2024

TODO: PR message

@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
Comment on lines +411 to +413
/// The boundary the stack is aligned to when the function is called.
/// `null` means the default for this calling convention.
incoming_stack_alignment: ?u64 = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this refers to the stack alignment right before the caller executes its call instruction, I think "before the function is called" from #21209 was better phrasing than "when the function is called."

/// `null` means the default for this calling convention.
incoming_stack_alignment: ?u64 = null,
/// The privilege mode.
mode: PrivilegeMode = .machine,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there should be a default mode, it's commonly either of them.

This commit begins implementing accepted proposal ziglang#21209 by making
`std.builtin.CallingConvention` a tagged union.

The stage1 dance here is a little convoluted. This commit introduces the
new type as `NewCallingConvention`, keeping the old `CallingConvention`
around. The compiler uses `std.builtin.NewCallingConvention`
exclusively, but when fetching the type from `std` when running the
compiler (e.g. with `getBuiltinType`), the name `CallingConvention` is
used. This allows a prior build of Zig to be used to build this commit.
The next commit will update `zig1.wasm`, and then the compiler and
standard library can be updated to completely replace
`CallingConvention` with `NewCallingConvention`.

The second half of ziglang#21209 is to remove `@setAlignStack`, which will be
implemented in another commit after updating `zig1.wasm`.
The old `CallingConvention` type is replaced with the new
`NewCallingConvention`. References to `NewCallingConvention` in the
compiler are updated accordingly. In addition, a few parts of the
standard library are updated to use the new type correctly.
This commit finishes implementing ziglang#21209 by removing the
`@setAlignStack` builtin in favour of `CallingConvention` payloads. The
x86_64 backend is updated to use the stack alignment given in the
calling convention (the LLVM backend was already updated in a previous
commit).

Resolves: ziglang#21209
This also includes some compiler and std changes to correct error
messages which weren't properly updated before.
…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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants