Stream: git-wasmtime

Topic: wasmtime / PR #3807 WIP: implicit conversions in ISLE.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 08:27):

cfallin opened PR #3807 from isle-implicit-conversions to main:

An initial sketch of the ISLE compiler implementation for implicit type conversions (#3753). I can clean it up a bit but this is the general shape at least.

Two implementation bits: (i) I opted to add conversions as their own IR node type, rather than try lowering these to extractors/constructors to share that code, as the operations are just a tiny bit different (not associated with a term, for example); and (ii) there is the option to denote a converter as returning a borrow or a value -- the former is useful in patterns, the latter may be necessary if the converted value does not already exist inside the original or in the context, and we also always clone to a value when in the right-hand side to avoid holding a borrow on the mut context.

I'll work a bit further ahead by trying to use this to simplify some of the lowering rules, to make sure it actually works as we hope it will (but any such use will be merged by a later PR).

cc @fitzgen for thoughts?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

Can this be inferred from whether the term used for converting is infallible?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

Is this assuming that the term used for converting is always an external Rust function? That seems like an unnecessary restriction to me.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

This also doesn't seem to differentiate between conversions in an extraction vs construction context, which we talked about in the issue.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

It seems strange to me that we need/have this for conversions but not for regular terms. Every place where we want to insert these implicit conversions works right now and doesn't have this borrow flag, so I feel like it shouldn't be necessary to add. I must be missing something here.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

This is unrelated to these particular changes, but long overdue nonetheless: can we write a #[test] that asserts that every file in isle_examples compiles okay? (At least through ISLE, testing the emitted code with rustc will be a bit harder/more tedious since it requires glue code).

And for the ones that are supposed to fail (I forget if we even have any of those) it would be nice if we had a comment at the top with a substring of the expected error message. These could be in a separate directory and also be handled in separate PRs.

But we really need some kind of testing story for this stuff, especially as we tweak the language and compiler.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 20:52):

fitzgen created PR review comment:

This can avoid a double lookup by doing something like

let old_entry = tyenv.converters.insert(...);
if let Some(old_entry) = old_entry {
    tyenv.report_error(...);
    continue;
}

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:01):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:01):

cfallin created PR review comment:

Yes, definitely agreed, this is probably as good a time as any to start!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:03):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:03):

cfallin created PR review comment:

Yes, if we go to a term-based design instead, for sure.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 21:07):

cfallin created PR review comment:

So this became necessary because conversion functions can be used on both the LHS and RHS, and on the LHS we want to deal in borrows as much as possible (as extractors work today) while on the RHS we're usually building entirely new structs. If we were to lower to terms instead then it wouldn't be necessary, as the extractor/constructor codegen already does basically the right thing here.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 22:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 22:06):

cfallin created PR review comment:

I don't think this is needed here: even if we do move to a terms-as-converters rather than explicit-converter-funcs design, we can look up the term and find an attached extractor or converter, so we don't need to record directionality here, I think.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 22:07):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 16 2022 at 22:07):

cfallin created PR review comment:

(commented below -- yes, I'll change this)

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 06:57):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:45):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:46):

cfallin created PR review comment:

Removed now, with term-based design.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:46):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:46):

cfallin created PR review comment:

Thanks! Used the entry API actually for a similarly cleaner single-lookup.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:47):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 07:47):

cfallin created PR review comment:

Done! We now have pass-tests, fail-tests, and link-tests; the last one checks that the generated code builds and links against the example driver/context trait impl.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 08:25):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 17 2022 at 20:32):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 18 2022 at 00:38):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 18 2022 at 03:13):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2022 at 21:50):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2022 at 21:52):

cfallin updated PR #3807 from isle-implicit-conversions to main.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2022 at 21:52):

cfallin has marked PR #3807 as ready for review.

Last updated: Dec 23 2024 at 12:05 UTC