wasmtime / PR #3807 Implicit type conversions in ISLE · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #3807 Implicit type conversions in ISLE

Wasmtime GitHub notifications bot (Feb 22 2022 at 22:07):

cfallin edited 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?

Wasmtime GitHub notifications bot (Feb 22 2022 at 22:19):

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

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:00):

fitzgen created PR review comment:

Thanks so much for writing these tests! <3

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:00):

fitzgen created PR review comment:

nitpick: missing trailing newline. and since I'm here, "for unit tests only" is kinda redundant since that is what dev-dependencies effectively are already.

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:00):

fitzgen created PR review comment:

Or just manually factor out the conversions into a single conversion with a let.

Bringing up CSE here is opening up more questions about implementation and how we guarantee that this is actually semantics-preserving than it answers in my mind, so sort of seems like a distraction from what is otherwise prescriptive documentation.

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:00):

fitzgen created PR review comment:

Can we have tests for when

the term is defined and has a constructor, but the conversion is in extractor position (and the term does not have an extractor)

vice versa with constructor position