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

Add builtin function @handle() #1297

Merged
merged 39 commits into from Aug 2, 2018

Conversation

Projects
None yet
2 participants
@kristate
Contributor

kristate commented Jul 27, 2018

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.

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"));

This comment has been minimized.

@andrewrk

andrewrk Jul 28, 2018

Member

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 :-)

src/ir.cpp Outdated
@@ -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 comment has been minimized.

@andrewrk

andrewrk Jul 28, 2018

Member

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.

src/ir.cpp Outdated
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);

This comment has been minimized.

@andrewrk

andrewrk Jul 28, 2018

Member

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

This comment has been minimized.

Member

andrewrk commented Jul 28, 2018

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.

@@ -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,

This comment has been minimized.

@kristate

kristate Jul 28, 2018

Contributor

@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.

This comment has been minimized.

@kristate

kristate Jul 28, 2018

Contributor

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

This comment has been minimized.

@andrewrk

andrewrk Jul 28, 2018

Member

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

This comment has been minimized.

@andrewrk

andrewrk Jul 28, 2018

Member

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

This comment has been minimized.

@kristate

kristate Jul 29, 2018

Contributor

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

This comment has been minimized.

Contributor

kristate commented Jul 28, 2018

@andrewrk please review :-)

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

@andrewrk

This comment has been minimized.

Member

andrewrk commented Jul 28, 2018

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

This comment has been minimized.

Member

andrewrk commented Jul 28, 2018

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

This comment has been minimized.

Contributor

kristate commented Jul 29, 2018

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

@kristate

This comment has been minimized.

Contributor

kristate commented Jul 29, 2018

@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

This comment has been minimized.

Member

andrewrk commented Jul 31, 2018

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

@andrewrk

This comment has been minimized.

Member

andrewrk commented Jul 31, 2018

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

This comment has been minimized.

Contributor

kristate commented Aug 2, 2018

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

kristate added some commits Jul 27, 2018

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

I removed/commented-out the assert checking for no errors since we now have some errors rendered.

kristate added some commits Jul 29, 2018

std/zig/parser_test.zig: update test to reflect that the promise symb…
…ol is no in scope with suspend;

Tracking Issue #1296 ;
test/cases/coroutine_await_struct.zig: update test to reflect that th…
…e promise symbol is no in scope with suspend;

Tracking Issue #1296 ;
test/cases/coroutines.zig: update test to reflect that the promise sy…
…mbol is no in scope with suspend;

Tracking Issue #1296 ;
test/compile_errors.zig: update test to reflect that the promise symb…
…ol is no in scope with suspend;

Tracking Issue #1296 ;
doc/langref.html.in: update docs to reflect that the promise symbol i…
…s no in scope with suspend;

Tracking Issue #1296 ;

@kristate kristate force-pushed the kristate:handle-builtin-issue1296 branch from d7123d2 to ff4a03f Aug 2, 2018

@kristate

This comment has been minimized.

Contributor

kristate commented Aug 2, 2018

@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.

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kristate kristate deleted the kristate:handle-builtin-issue1296 branch Aug 2, 2018

@kristate kristate restored the kristate:handle-builtin-issue1296 branch Aug 3, 2018

@kristate kristate deleted the kristate:handle-builtin-issue1296 branch Aug 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment