AnyDSL / artic

The AlteRnaTive Impala Compiler
MIT License
25 stars 6 forks source link

Passing continuation to function crashes compiler #7

Open michael-kenzel opened 2 years ago

michael-kenzel commented 2 years ago

The following code will reproduce the issue:

fn test() {
    let blub = @|cont: fn()->()| {};

    while (true) {
        blub(continue)
    }
}

Compiling the above code results in:

terminate called after throwing an instance of 'std::bad_array_new_length'
  what():  std::bad_array_new_length

I assume this may be caused by the incorrect return type in the function type of the parameter the continuation is passed to.

One workaround seems to be wrapping the continuation in a lambda:

fn test() {
    let blub = @|cont: fn()->()| {};

    while (true) {
        blub(@||continue())
    }
}
Hugobros3 commented 2 years ago

So this is actually type-sound, as the type of continue is fn() -> !, with ! being the bottom (or more specifically, no-return) type. Because continue never returns, it also can be validly placed in contexts excepting some return value, as it will never return a wrong type (since it never returns!).

We run into an issue however when trying to emit Thorin code for this, because thorin encodes returns with another parameter, so the type of the cont (the param of blub) is fn(fn() -> !) -> !, but we're giving it a fn() -> !, and so the generated thorin code doesn't type.

fn blub (cont: fn(fn() -> !) -> !, ret: fn() -> !) = ret()

fn while_loop_continue() = while_loop_head()

fn test_while_...() = blub(while_loop_continue)

The solution ? Well Artic actually has some code already to deal with this edge case, but it isn't used in all contexts it seems, in particular when auto-casting it doesn't get called, so this is hopefully a simple refactor to fix.

Hugobros3 commented 2 years ago

Wrote a fix (https://github.com/AnyDSL/artic/commit/6cf219d51bd49336e3cf6b1b7e90445c1765e8d7), the output now looks sensible:

test_6: fn[mem, fn[mem]] = {
    test_6: fn[mem, fn[mem]] = while_head_15(test_7[test_6])
}

while_head_15: fn[mem] = {
    while_head_15: fn[mem] = branch_true_25()
}

branch_true_25: fn[] = {
    branch_true_25: fn[] = while_body_23(while_head_16[while_head_15])
}

while_body_23: fn[mem] = {
    while_body_23: fn[mem] = blub_10(while_body_24[while_body_23], _27, cont_30)
}

blub_10: fn[mem, fn[mem, fn[mem]], fn[mem]] = {
    blub_10: fn[mem, fn[mem, fn[mem]], fn[mem]] = ret_13[blub_10](_11[blub_10])
}

_27: fn[mem, fn[mem]] = {
    _27: fn[mem, fn[mem]] = while_continue_19(_28[_27])
}

cont_30: fn[mem] = {
    cont_30: fn[mem] = while_head_15(cont_31[cont_30])
}

while_continue_19: fn[mem] = {
    while_continue_19: fn[mem] = while_head_15(while_continue_20[while_continue_19])
}
madmann91 commented 2 years ago

Is this fixed? Can we close the issue?

Hugobros3 commented 1 year ago

I think so, I did write a fix, sadly the specifics are fuzzy to me. Since there was no activity afterwards, I guess that did it!

Another issue can be opened if a similar problem arises.

michael-kenzel commented 1 year ago

I'm afraid the problem does not appear to be resolved completely as I've run into it again.

The following example

fn ohnoes(throw: fn(&[u8]) -> ()) {
    throw("");
}

#[export]
fn test() -> () {
    let throw = return;
    let handle_failure = @|msg: &[u8]| {
        throw()
    };

    ohnoes(handle_failure);
}

results in

src/emit.cpp:776: void artic::Emitter::bind(const artic::ast::IdPtrn&, const thorin::Def*): Assertion `id_ptrn.type->convert(*this) == value->type()' failed.

And this slight variation of the example:

fn ohnoes(throw: fn() -> ()) {
    throw();
}

#[export]
fn test() -> () {
    let throw = return;
    let handle_failure = @|| {
        throw()
    };

    ohnoes(handle_failure);
}

triggers

terminate called after throwing an instance of 'std::bad_array_new_length'
  what():  std::bad_array_new_length
Hugobros3 commented 1 year ago

I just debugged this, basically handle_failure looks like a basic block to Artic because it does not return (it calls throw that does not return either), but it is later assumed to be a function and that causes an off-by-one error in tuple_from_params. I have a basic fix that I just sent, but eventually I plan to add a proper distinction between functions and basic blocks that doesn't just look at whether they return something or not. (I want to support functions that don't return as first-class functions)

michael-kenzel commented 1 year ago

I'm afraid this now seems to break the following code that's part of the runtime (reduced sample):

#[import(cc = "thorin")] fn opencl(_dev: i32, _grid: (i32, i32, i32), _block: (i32, i32, i32), _body: fn() -> ()) -> ();

struct WorkItem {}

struct Accelerator {
    exec : fn(fn(WorkItem) -> ()) -> fn((i32, i32, i32), (i32, i32, i32)) -> (), // fn(grid, block)->()
}

fn @opencl_accelerator(dev: i32) = Accelerator {
    exec = @|body| |grid, block| {
        opencl(dev, grid, block, || @body(WorkItem {}))
    }
};

produces

/src/emit.cpp:776: void artic::Emitter::bind(const artic::ast::IdPtrn&, const thorin::Def*): Assertion `id_ptrn.type->convert(*this) == value->type()' failed.
PearCoding commented 1 year ago

I also get an Access Violation with the new changes while compiling code. I would suggest taking the last commit back until more information is available?

Hugobros3 commented 1 year ago

Sorry, that's what I get for pushing a "trivial" fix, on a Friday no less! I rolled back the patch and pushed again.

I will open another issue on Monday to keep track of the wider problem.