Introduce callconv annotation by LemonBoy · Pull Request #3977 · ziglang/zig

LemonBoy · 2019-12-24T01:22:21Z

Done according to the accepted proposal, zig fmt has been patched up to use the new notation when possible and the whole stdlib has been already updated.
A few new calling conventions have been introduced, you can now write your interrupt handlers (for x86 & AVR) with pure Zig code.

Missing:

Integrate with translate-c-2

Not critical but possibly wanted:

Validate export/extern/callconv combinations
Checks to prevent the use of unsupported CCs for a given target arch

A few open questions:

Some CCs are architecture specific, do we want to add a prefix to those (eg. Fastcall -> X86_Fastcall) ?
Right now we "group" all the CCs under the same name (eg. AVR interrupt & X86 interrupt are both under the Interrupt entry) and are chosen according to the target architecture. Is this ok or do we want to split them into independent entities?

I'm submitting this as-is to see if the CI is green, ~~in case this is merged please rebase it~~.

Closes #661

daurnimator · 2019-12-24T02:22:18Z

lib/std/mutex.zig

-else if (builtin.os == .windows) 
-    // https://locklessinc.com/articles/keyed_events/
+else if (builtin.os == .windows)
+// https://locklessinc.com/articles/keyed_events/


This doesn't look right to me. Created #3978

andrewrk

Some CCs are architecture specific, do we want to add a prefix to those (eg. Fastcall -> X86_Fastcall) ?

Makes sense to me, let's do it. For "fastcall" it is especially nice because I don't want people to be confused and think "fastcall" is actually somehow "faster", it's more like "quicksort" is to sorting. So it's nice to have the architecture there to imply that this is a calling convention, not a performance thing.

Right now we "group" all the CCs under the same name (eg. AVR interrupt & X86 interrupt are both under the Interrupt entry) and are chosen according to the target architecture. Is this ok or do we want to split them into independent entities?

Hmm, good question. I really can't think of a reason to favor one or the other. With "C calling convention" we already have precedent for a calling convention that means different things on different architecture, so "interrupt" meaning "the architecture's native interrupt calling convention" seems reasonable to me. I defer to your judgment on this one.

This PR is already close enough to mergeable that I would feel comfortable with a hand-off. So let me know when you're done with it, and I'll make sure it lands swiftly in master branch.

andrewrk · 2019-12-31T04:03:10Z

src/analyze.cpp

-    const char *cc_str = calling_convention_fn_type_str(fn_type->data.fn.fn_type_id.cc);
-    buf_appendf(&fn_type->name, "%s", cc_str);
+    if (fn_type->data.fn.fn_type_id.cc == CallingConventionC)
+        buf_append_str(&fn_type->name, "extern ");


I was thinking we could drop extern for the C calling convention, once we have callconv. Any reason to keep using extern for C calling convention functions?

Decoupling extern from the CC is definitely a good idea, I tried not to change the status quo too much in this PR so I think I'll leave the breaking changes to you :)

andrewrk · 2019-12-31T04:05:23Z

src/all_types.hpp

+    bool is_nakedcc;
+    bool is_stdcallcc;
+    bool is_async;


Feel free to delete nakedcc and stdcallcc keywords from stage1. (If you don't do it, I'll do it.) I'm happy to tell people to use the auto-fixing you added to zig fmt in order to upgrade code to the latest version.

I'll open a separate proposal to remove support for async fn in favor of callconv(.Async) for uniformity.

Ditto, if you want to drop nakedcc and stdcallcc in this release cycle go for it! The less legacy cruft the better.

lib/std/zig/render.zig

andrewrk · 2019-12-31T04:09:11Z

src/analyze.cpp

            if (field_type == nullptr) {
                struct_type->data.structure.resolve_status = ResolveStatusInvalid;
-                return err;
+                return ErrorSemanticAnalyzeFail;


yikes! nice catch.

andrewrk · 2019-12-31T04:15:09Z

src/codegen.cpp

+        case CallingConventionVectorcall:
+            if (g->zig_target->arch == ZigLLVM_x86)
+                return LLVMX86VectorCallCallConv;
+        // XXX Enable this when the C API exports this enum member too


We should work around this with zig_llvm.h/zig_llvm.cpp. The official LLVM C API always lags behind the C++ API.

In fact:

zig/src/zig_llvm.h

Lines 74 to 75 in 86ba8c0

ZIG_EXTERN_C LLVMValueRef ZigLLVMBuildCall(LLVMBuilderRef B, LLVMValueRef Fn, LLVMValueRef *Args,

unsigned NumArgs, unsigned CC, enum ZigLLVM_CallAttr attr, const char *Name);

zig/src/zig_llvm.cpp

Lines 275 to 297 in 86ba8c0

LLVMValueRef ZigLLVMBuildCall(LLVMBuilderRef B, LLVMValueRef Fn, LLVMValueRef *Args,

unsigned NumArgs, unsigned CC, ZigLLVM_CallAttr attr, const char *Name)

{

CallInst *call_inst = CallInst::Create(unwrap(Fn), makeArrayRef(unwrap(Args), NumArgs), Name);

call_inst->setCallingConv(CC);

switch (attr) {

case ZigLLVM_CallAttrAuto:

break;

case ZigLLVM_CallAttrNeverTail:

call_inst->setTailCallKind(CallInst::TCK_NoTail);

break;

case ZigLLVM_CallAttrNeverInline:

call_inst->addAttribute(AttributeList::FunctionIndex, Attribute::NoInline);

break;

case ZigLLVM_CallAttrAlwaysTail:

call_inst->setTailCallKind(CallInst::TCK_MustTail);

break;

case ZigLLVM_CallAttrAlwaysInline:

call_inst->addAttribute(AttributeList::FunctionIndex, Attribute::AlwaysInline);

break;

}

return wrap(unwrap(B)->Insert(call_inst));

}

Looks like all we're missing is a copy-paste of LLVM's calling convention enum, with some static_asserts in zig_llvm.cpp to make sure the values don't change. See src/zig_clang.cpp for inspiration.

I've sent a patch that hopefully will get in by the time LLVM 10 is out. But moving away from the C API is also a good idea.

LemonBoy

Makes sense to me, let's do it. For "fastcall" it is especially nice because I don't want people to be confused and think "fastcall" is actually somehow "faster", it's more like "quicksort" is to sorting. So it's nice to have the architecture there to imply that this is a calling convention, not a performance thing.

If we go for the prefix route then we should also split the common CCs (Vectorcall becomes X86_Vectorcall and AArch64_Vectorcall, same for Interrupt and so on). IMO it's fine to leave the enums un-prefixed and merged with some hints in the documentation about what to expect.

andrewrk · 2019-12-31T18:43:37Z

IMO it's fine to leave the enums un-prefixed and merged with some hints in the documentation about what to expect.

👍

LemonBoy · 2019-12-31T22:44:55Z

This is ready to go on my end, I'll send a follow-up PR to make compiler-rt use the AAPCS cc for the AEABI builtins once this is merged.

Returning the uninitialized/stale error condition made the compiler turn a blind eye to some problems.

daurnimator reviewed Dec 24, 2019

View reviewed changes

LemonBoy force-pushed the cc-work branch 7 times, most recently from 63252ce to 4fbe848 Compare December 30, 2019 10:24

andrewrk reviewed Dec 31, 2019

View reviewed changes

LemonBoy commented Dec 31, 2019

View reviewed changes

LemonBoy force-pushed the cc-work branch from 21eb18c to b6f3000 Compare December 31, 2019 18:38

LemonBoy added 8 commits January 2, 2020 18:53

Implement the callconv() annotation

563d9eb

Catch more errors during the type resolution phase

271fc6a

Returning the uninitialized/stale error condition made the compiler turn a blind eye to some problems.

Integrate callconv into translate-c-2

0ec64d4

translate-c tests

08a26fe

Implement Thiscall CC

0ccac79

Add transform test

e99209b

Use the LLVM C++ API

435528a

Fix more tests

d09bd3d

LemonBoy force-pushed the cc-work branch from b6f3000 to d09bd3d Compare January 2, 2020 18:01

LemonBoy mentioned this pull request Jan 4, 2020

Add more compiler-rt functions for ARM platform #4073

Merged

andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Jan 6, 2020

andrewrk merged commit d09bd3d into ziglang:master Jan 6, 2020

LemonBoy mentioned this pull request Jan 20, 2020

Replace extern with extern(...) and layout(...) #4245

Closed

tadeokondrak mentioned this pull request May 4, 2020

Remove syntax redundant with callconv #5268

Merged

	ZIG_EXTERN_C LLVMValueRef ZigLLVMBuildCall(LLVMBuilderRef B, LLVMValueRef Fn, LLVMValueRef *Args,
	unsigned NumArgs, unsigned CC, enum ZigLLVM_CallAttr attr, const char *Name);

	LLVMValueRef ZigLLVMBuildCall(LLVMBuilderRef B, LLVMValueRef Fn, LLVMValueRef *Args,
	unsigned NumArgs, unsigned CC, ZigLLVM_CallAttr attr, const char *Name)
	{
	CallInst *call_inst = CallInst::Create(unwrap(Fn), makeArrayRef(unwrap(Args), NumArgs), Name);
	call_inst->setCallingConv(CC);
	switch (attr) {
	case ZigLLVM_CallAttrAuto:
	break;
	case ZigLLVM_CallAttrNeverTail:
	call_inst->setTailCallKind(CallInst::TCK_NoTail);
	break;
	case ZigLLVM_CallAttrNeverInline:
	call_inst->addAttribute(AttributeList::FunctionIndex, Attribute::NoInline);
	break;
	case ZigLLVM_CallAttrAlwaysTail:
	call_inst->setTailCallKind(CallInst::TCK_MustTail);
	break;
	case ZigLLVM_CallAttrAlwaysInline:
	call_inst->addAttribute(AttributeList::FunctionIndex, Attribute::AlwaysInline);
	break;
	}
	return wrap(unwrap(B)->Insert(call_inst));
	}

Uh oh!

Conversation

LemonBoy commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LemonBoy left a comment

Choose a reason for hiding this comment

Uh oh!

andrewrk commented Dec 31, 2019

Uh oh!

LemonBoy commented Dec 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LemonBoy commented Dec 24, 2019 •

edited

Loading