Stream: git-wasmtime

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

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.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

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

Wasmtime GitHub notifications bot (Sep 14 2022 at 00:42):

cfallin requested fitzgen for a review on PR #4908.

Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 17:56):

fitzgen submitted PR review.

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.

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.

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:

• It is general enough to subsume vector returns since you can always use std::vec::Iter as your iterator type.
• It doesn't require computing all the values ahead of time, which can be more efficient if we ever allow short circuiting.
• It simplifies authoring the glue code (only one case for multi things instead of two) which makes it easier for users to grok the compilation model.

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.

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.

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

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:28):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 19:18):

jameysharp submitted PR review.

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.

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.

Wasmtime GitHub notifications bot (Sep 14 2022 at 21:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 19 2022 at 21:25):

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

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.

Wasmtime GitHub notifications bot (Sep 19 2022 at 21:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 19 2022 at 21:28):

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

Wasmtime GitHub notifications bot (Sep 21 2022 at 23:04):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 23:06):

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

Wasmtime GitHub notifications bot (Sep 21 2022 at 23:28):

cfallin has enabled auto merge for PR #4908.

Wasmtime GitHub notifications bot (Sep 21 2022 at 23:36):

cfallin merged PR #4908.

Last updated: Nov 22 2024 at 17:03 UTC