Add builtin function @handle() #1297

kristate · 2018-07-27T10:27:48Z

closes #1296 ;

Define Builtin @handle();
Check if within Async block
I am using executable->fn_entry->type_entry->data.fn.fn_type_id.cc == CallingConventionAsync to check this condition.
Return promise in codegen
Using @llvm.coro.frame
Remove old semantic analysis code for the feature we are removing
remove suspend |handle| {}
Remove the stage1 parser code for it
Remove the stage2 parser code for it
Write documentation

@andrewrk Andrew, I took a crack at this because I want to learn more about zig's internals and based it off of @frameAddress(). I learned a lot from this article.

If you can help me understand the rest, I can start helping with more of the internals. :-)

Thanks

andrewrk

Thank you for starting this effort. I hope these comments help.

andrewrk · 2018-07-28T05:32:22Z

src/codegen.cpp

+        executable->fn_entry->type_entry->data.fn.fn_type_id.cc == CallingConventionAsync;
+
+    if (!is_async || !executable->coro_handle) {
+      add_node_error(g, instruction->base.source_node, buf_sprintf("@handle() in non-async function"));


in render functions in codegen.cpp it is too late - we need to put this semantic analysis and compile error in ir.cpp in the analyze_instruction_handle function. That's why that assert was there that you commented out :-)

andrewrk · 2018-07-28T05:36:12Z

src/ir.cpp

@@ -4448,6 +4463,8 @@ static IrInstruction *ir_gen_builtin_fn_call(IrBuilder *irb, Scope *scope, AstNo
            return ir_lval_wrap(irb, scope, ir_build_return_address(irb, scope, node), lval);
        case BuiltinFnIdFrameAddress:
            return ir_lval_wrap(irb, scope, ir_build_frame_address(irb, scope, node), lval);
+        case BuiltinFnIdHandle:
+            return ir_lval_wrap(irb, scope, ir_build_handle(irb, scope, node), lval);


This is probably actually the best place to put the compile error for @handle() used outside an async function. Search for return expression outside function definition to see an example.

andrewrk · 2018-07-28T05:38:01Z

src/ir.cpp

+static TypeTableEntry *ir_analyze_instruction_handle(IrAnalyze *ira, IrInstructionHandle *instruction) {
+    ir_build_handle_from(&ira->new_irb, &instruction->base);
+
+    TypeTableEntry *promise_type = get_promise_type(ira->codegen, nullptr);


Here, I think that @handle() should return a promise->T rather than just a promise, because we know the return type of the async function. If you look at the type of the variable that suspend blocks provide, it does this. You can see an example of this in ir_analyze_instruction_coro_begin.

andrewrk · 2018-07-28T05:40:14Z

Here are some more checklist items:

remove old semantic analysis code for the feature we are removing
remove the stage1 parser code for it
remove the stage2 parser code for it

The self-hosted compiler does not have coroutine functionality yet, (but it does have a full parser) so you don't need to port this to stage2.

kristate · 2018-07-28T07:06:43Z

src/codegen.cpp

@@ -4146,6 +4146,12 @@ static LLVMValueRef ir_render_frame_address(CodeGen *g, IrExecutable *executable
    return LLVMBuildCall(g->builder, get_frame_address_fn_val(g), &zero, 1, "");
 }

+static LLVMValueRef ir_render_handle(CodeGen *g, IrExecutable *executable,


@andrewrk I am pretty sure that the coro_handle is stored in IrExecutable, but I am wondering how to expose it as an LLVMValueRef ? It seems to have no llvm_value.

Solved it by LLVMBuildBitCasting executable->fn_entry->ir_executable.coro_handle->other

Hmm. That might be ok. A more straightforward solution would probably be to use @llvm.coro.frame: http://llvm.org/docs/Coroutines.html#llvm-coro-frame-intrinsic

To be clear, I think you can leave this as is, but if it causes problems, we'll know to try this other approach.

Cool, I replaced it with the llvm.coro.frame which I think will give better separation between IR and codegen. I did look through the docs several times, and I am not sure why I didn't pickup on it. Thanks!

kristate · 2018-07-28T15:15:42Z

@andrewrk please review :-)

Also, please let me know what you meant by "Remove old semantic analysis code"

andrewrk · 2018-07-28T15:23:22Z

this code needs to be modified so that we're not putting a variable in scope:

        Buf *promise_symbol_name = node->data.suspend.promise_symbol->data.symbol_expr.symbol;
        Scope *child_scope;
        if (!buf_eql_str(promise_symbol_name, "_")) {
            VariableTableEntry *promise_var = ir_create_var(irb, node, parent_scope, promise_symbol_name,
                    true, true, false, const_bool_false);
            ir_build_var_decl(irb, parent_scope, node, promise_var, nullptr, nullptr, irb->exec->coro_handle);
            child_scope = promise_var->child_scope;
        } else {
            child_scope = parent_scope;
        }

Once you update the parser you'll get a compile error for this.

andrewrk · 2018-07-28T15:35:33Z

I think it looks good so far! Once you remove the old syntax, you'll probably have to update a few places in the standard library. If those tests still pass, that'll be a good test for this patch.

kristate · 2018-07-29T05:15:16Z

Pretty sure I got everything -- running tests locally and then I will push.

kristate · 2018-07-29T08:24:18Z

@andrewrk I am worried that resume @handle(); is not working correctly. I created a test for it (and the tests pass), but in some cases (lock.zig + loop.zig) it seems that the value is being optimized out. Setting a var before resume works in all cases.

This sometimes fails:

suspend {
    resume @handle();
}

This will always work:

suspend {
    var h: promise = @handle();
    resume h;
}

andrewrk · 2018-07-31T02:24:03Z

I'll have a look at the failure and see if I can figure out why it's happening

andrewrk · 2018-07-31T15:47:19Z

it seems that the value is being optimized out.

This actually makes me think that my suggestion to use @llvm.coro.frame was a mistake. The docs say

This intrinsic is lowered to refer to the coro.begin instruction. This is a frontend convenience intrinsic that makes it easier to refer to the coroutine frame.

It seems like a bug that LLVM would do this, and then not spill it and restore it correctly. But anyway we can work around it by having Zig store the handle in a variable secretly (which is what was happening with the old syntax).

I'm happy to take this pull request from here and drive it to completion. Would you like me to do that, or would you like to keep working on it yourself?

kristate · 2018-08-02T05:51:52Z

@andrewrk It looks like you reworked coroutines, so I will resolve this branch with the master and then test again.

Tracking Issue ziglang#1296 ;

Tracking Issue ziglang#1296 ; I removed/commented-out the assert checking for no errors since we now have some errors rendered.

@andrewrk

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

@andrewrk

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

@andrewrk

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

@andrewrk

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

Tracking Issue ziglang#1296 ;

…ion; Tracking Issue ziglang#1296 ;

Tracking Issue ziglang#1296 ;

…ndle

…ndle(); Tracking Issue ziglang#1296 ;

(); Tracking Issue ziglang#1296 ;

…(); Tracking Issue ziglang#1296 ;

…); Tracking Issue ziglang#1296 ;

Tracking Issue ziglang#1296 ;

…ol is no in scope with suspend; Tracking Issue ziglang#1296 ;

…e promise symbol is no in scope with suspend; Tracking Issue ziglang#1296 ;

…mbol is no in scope with suspend; Tracking Issue ziglang#1296 ;

@handle

…d with @handle(); Tracking Issue ziglang#1296 ;

…ol is no in scope with suspend; Tracking Issue ziglang#1296 ;

…s no in scope with suspend; Tracking Issue ziglang#1296 ;

Tracking Issue ziglang#1296 ;

kristate · 2018-08-02T08:54:15Z

@andrewrk I think that the resume @handle bug has gone away after your rework! 96a94e7 uses resume @handle and if it passes, I think we're all clear to merge this PR.

andrewrk reviewed Jul 28, 2018

View reviewed changes

kristate commented Jul 28, 2018

View reviewed changes

kristopher tate added 19 commits August 2, 2018 16:50

src/all_types.hpp: add enums for Handle Builtin;

e79c913

Tracking Issue ziglang#1296 ;

src/ir.cpp: wire-up IR for handle builtin;

a9ea22d

Tracking Issue ziglang#1296 ;

src/codegen.cpp: base handle builtin on @frameAddress();

cd18186

Tracking Issue ziglang#1296 ;

src/ir_print.cpp: support @handle();

da5f3d5

Tracking Issue ziglang#1296 ;

test/cases/couroutines.zig: test @handle();

9366a58

Tracking Issue ziglang#1296 ;

src/codegen.cpp: return null if calling convention is not async;

a2e5691

Tracking Issue ziglang#1296 ;

src/codegen.cpp: add/throw error for @handle() in a non async context;

81f4636

Tracking Issue ziglang#1296 ; I removed/commented-out the assert checking for no errors since we now have some errors rendered.

src/codegen.cpp: remove add_node_error from ir_render_handle;

0ee6502

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

src/codegen.cpp: reassert that there are no generated errors in codegen;

db362be

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

src/ir.cpp: add/throw error for @handle() in a non async context;

c1a3b0c

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

test/cases/coroutines.zig: remove dummy assert used for testing;

1f0040d

src/ir.cpp: return promise->T instead of promise;

fcf53b3

Tracking Issue ziglang#1296 ; Thanks @andrewrk ;

src/ir.cpp: don't allow @handle() outside of a function;

a8ea236

Tracking Issue ziglang#1296 ;

src/codegen.cpp: return promise instead of null promise;

104bdb0

Tracking Issue ziglang#1296 ;

test/compile_errors.zig: @handle() called outside of function definit…

c546f75

…ion; Tracking Issue ziglang#1296 ;

test/compile_errors.zig: @handle() in non-async function

13ec5db

Tracking Issue ziglang#1296 ;

src/codegen.cpp: @handle(): replace hacky ref chain with llvm intrinsic;

92cb330

Tracking Issue ziglang#1296 ;

src/all_types.hpp: remove promise_symbol from suspend;

ca1b356

Tracking Issue ziglang#1296 ;

src/ast_render.cpp: remove promise_symbol from suspend;

5e5685c

Tracking Issue ziglang#1296 ;

kristopher tate added 14 commits August 2, 2018 16:50

src/ir.cpp: remove promise_symbol from suspend;

b3cd65d

Tracking Issue ziglang#1296 ;

src/parser.cpp: remove promise_symbol from suspend;

d3f6289

Tracking Issue ziglang#1296 ;

std/event/channel.zig: remove promise_symbol from suspend and use @ha…

29057e5

…ndle(); Tracking Issue ziglang#1296 ;

std/event/future.zig: remove promise_symbol from suspend and use @handle

244a7fd

(); Tracking Issue ziglang#1296 ;

std/event/group.zig: remove promise_symbol from suspend and use @handle…

b4ff464

…(); Tracking Issue ziglang#1296 ;

std/event/lock.zig: remove promise_symbol from suspend and use @handle(…

efec3a0

…); Tracking Issue ziglang#1296 ;

std/event/loop.zig: remove promise_symbol from suspend and use @handle(…

a3705b4

…); Tracking Issue ziglang#1296 ;

std/event/tcp.zig: remove promise_symbol from suspend and use @handle();

9fe140a

Tracking Issue ziglang#1296 ;

std/zig/parser_test.zig: update test to reflect that the promise symb…

bc032a8

…ol is no in scope with suspend; Tracking Issue ziglang#1296 ;

test/cases/coroutine_await_struct.zig: update test to reflect that th…

79792a3

…e promise symbol is no in scope with suspend; Tracking Issue ziglang#1296 ;

test/cases/coroutines.zig: update test to reflect that the promise sy…

3241ada

…mbol is no in scope with suspend; Tracking Issue ziglang#1296 ;

test/cases/coroutines.zig: test for immediate resume inside of suspen…

9b3cebc

…d with @handle(); Tracking Issue ziglang#1296 ;

test/compile_errors.zig: update test to reflect that the promise symb…

51955a5

…ol is no in scope with suspend; Tracking Issue ziglang#1296 ;

doc/langref.html.in: update docs to reflect that the promise symbol i…

ff4a03f

…s no in scope with suspend; Tracking Issue ziglang#1296 ;

kristate force-pushed the handle-builtin-issue1296 branch from d7123d2 to ff4a03f Compare August 2, 2018 08:05

kristopher tate added 6 commits August 2, 2018 17:11

src/parser.cpp: fix typo from rebase;

5de9242

doc/langref.html.in: update suspend example with @handle();

915e321

Tracking Issue ziglang#1296 ;

test/cases/coroutines.zig: update suspend to use @handle();

9bed23f

Tracking Issue ziglang#1296 ;

test/cases/cancel.zig: update suspend to use @handle();

9b890d7

Tracking Issue ziglang#1296 ;

doc/langref.html.in: add builtin @handle() to docs;

ac0a87d

Tracking Issue ziglang#1296 ;

std/event: directly return @handle();

96a94e7

Tracking Issue ziglang#1296 ;

kristate changed the title ~~[WIP] builtin function @handle()~~ Add builtin function @handle() Aug 2, 2018

andrewrk merged commit 96a94e7 into ziglang:master Aug 2, 2018

kristate deleted the handle-builtin-issue1296 branch August 2, 2018 18:19

kristate restored the handle-builtin-issue1296 branch August 3, 2018 06:07

kristate deleted the handle-builtin-issue1296 branch August 3, 2018 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add builtin function @handle() #1297

Add builtin function @handle() #1297

kristate commented Jul 27, 2018 •

edited

andrewrk left a comment

andrewrk Jul 28, 2018

andrewrk Jul 28, 2018

andrewrk Jul 28, 2018

andrewrk commented Jul 28, 2018 •

edited

kristate Jul 28, 2018

kristate Jul 28, 2018

andrewrk Jul 28, 2018

andrewrk Jul 28, 2018

kristate Jul 29, 2018

kristate commented Jul 28, 2018 •

edited

andrewrk commented Jul 28, 2018

andrewrk commented Jul 28, 2018

kristate commented Jul 29, 2018

kristate commented Jul 29, 2018

andrewrk commented Jul 31, 2018

andrewrk commented Jul 31, 2018 •

edited

kristate commented Aug 2, 2018

kristate commented Aug 2, 2018

Add builtin function @handle() #1297

Add builtin function @handle() #1297

Conversation

kristate commented Jul 27, 2018 • edited

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk commented Jul 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristate commented Jul 28, 2018 • edited

andrewrk commented Jul 28, 2018

andrewrk commented Jul 28, 2018

kristate commented Jul 29, 2018

kristate commented Jul 29, 2018

andrewrk commented Jul 31, 2018

andrewrk commented Jul 31, 2018 • edited

kristate commented Aug 2, 2018

kristate commented Aug 2, 2018

kristate commented Jul 27, 2018 •

edited

andrewrk commented Jul 28, 2018 •

edited

kristate commented Jul 28, 2018 •

edited

andrewrk commented Jul 31, 2018 •

edited