Stream: git-wasmtime

Topic: wasmtime / issue #5174 cranelift-isle: Factor out rule/pa...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 03:28):

github-actions[bot] commented on issue #5174:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Nov 02 2022 at 19:10):

jameysharp commented on issue #5174:

I've thought of one hypothetical case where this might change behavior: if the top-level pattern on the left-hand side of some rule is a bind-pattern.

In the current implementation, ir::lower_rule calls ruledata.lhs.root_term() to determine where to get the generated function's parameters from. It then calls gen_pattern on the ruledata.lhs.

Pattern::root_term recurses on the subpattern if it's called on a Pattern::BindPattern, until it finds a Pattern::Term. But after this PR, Rule::visit panics if the top-level pattern is a bind-pattern instead of a term.

I think the existing behavior isn't useful, because you can't _do_ anything with a name bound to the root term. The root term doesn't have a Rust representation that you can pattern match on or use in expressions. The current implementation of gen_pattern silently ignores the attempt to bind the name, so if you later try to use that name, it'll panic saying "Variable should already be bound".

So I think this is a reasonable behavior "change" in that it'll report a meaningless ISLE construct sooner. And this doesn't happen anywhere in our test suite or our existing cranelift-codegen rules (or we'd see this panic in CI).

I think we should go a step further and forbid bind-patterns in Pattern::root_term, as well as anywhere else that might currently permit them at the root of Rule::lhs.

Last updated: Oct 23 2024 at 20:03 UTC