Stream: git-wasmtime

Topic: wasmtime / PR #4908 ISLE: add support for multi-extractor...


view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 00:42):

cfallin opened PR #4908 from isle-multi-extractors to main:

This support allows for rules that process multiple matching values per extractor call on the left-hand side, and as a result, can produce multiple values from the constructor whose body they define.

This is useful in situations where we are matching on an input data structure that can have multiple "nodes" for a given value or ID, for example in an e-graph.

This is the first piece of the implementation of bytecodealliance/rfcs#27.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 00:42):

cfallin requested fitzgen for a review on PR #4908.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen created PR review comment:

These comments don't seem to reflect the current truth. Both extractors and constructors seem to be using ContextIter now, based on the test cases.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen created PR review comment:

When is this used? I don't see any examples in the test cases, so if this isn't dead code now, then we should add test cases.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen created PR review comment:

Ah okay here is an example of returning a vector.

But yeah, I think we should only use ContextIter:

As far as internal constructors go, we could have a blanket implementation of ContextIter for Vec<T> and SmallVec<T> so that the generated code doesn't need to change.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen created PR review comment:

Also, if this is still used, it seems like we could just have ContextIter and it can generalize and subsume the Vec case? That would simplify things a bit, especially for programmer mental model of what a multi compiles down into, since there won't be different cases.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 18:28):

cfallin created PR review comment:

I would love for that to work out, but the problem here is ownership: there is not necessarily any other owner for a newly-generated set of return values from a multi-constructor. Take for example a top-level "simplify" entry point that returns multiple equivalent expressions for a given input expression: it will need to return a vector of expression/node/eclass IDs, but this vec doesn't exist anywhere else (and stashing it in a side-table just to return an iterator over it would be awkward because it doesn't need to persist beyond the postprocessing we do on it).

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 18:28):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 19:18):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 19:18):

jameysharp created PR review comment:

I haven't read the surrounding code, but in general, an Iterator can itself own the data over which it's iterating. In particular, Vec::into_iter returns an iterator that owns the vector and will drop it when the iterator is dropped.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 21:19):

cfallin created PR review comment:

Ah, that's a good point! I'll rework in this direction.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 14 2022 at 21:19):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2022 at 21:25):

cfallin updated PR #4908 from isle-multi-extractors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2022 at 21:27):

cfallin created PR review comment:

Updated just now. It's a little awkward with the ContextIter adapters (this is necessary to allow the lowering trait to provide multi-etor iterators without holding a borrow on the whole Context) but it seems to work.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2022 at 21:27):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 19 2022 at 21:28):

cfallin updated PR #4908 from isle-multi-extractors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2022 at 23:04):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2022 at 23:06):

cfallin updated PR #4908 from isle-multi-extractors to main.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2022 at 23:28):

cfallin has enabled auto merge for PR #4908.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 21 2022 at 23:36):

cfallin merged PR #4908.


Last updated: Dec 23 2024 at 12:05 UTC