Stream: git-wasmtime

Topic: wasmtime / PR #5174 cranelift-isle: Factor out rule/patte...

Wasmtime GitHub notifications bot (Nov 02 2022 at 01:57):

jameysharp requested elliottt for a review on PR #5174.

Wasmtime GitHub notifications bot (Nov 02 2022 at 01:57):

jameysharp opened PR #5174 from rule-visitor to main:

This shouldn't change behavior at all, but makes some rather tricky analysis available to other users besides the current IR.

<!--

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 (Nov 02 2022 at 19:21):

jameysharp updated PR #5174 from rule-visitor to main.

Wasmtime GitHub notifications bot (Nov 02 2022 at 22:14):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 02 2022 at 22:14):

elliottt created PR review comment:

Why have the RuleVisitor extend PatternVisitor but have a ExprVisitor associated type? Would it be reasonable to have a PatternVisitor associated type here instead?

Having both types be associated types would I think force the interface used in lower_rule to be a bit more consistent -- right now the expression sequence is returned while the pattern sequence is an in/out param.

Wasmtime GitHub notifications bot (Nov 02 2022 at 22:14):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 00:05):

jameysharp updated PR #5174 from rule-visitor to main.

Wasmtime GitHub notifications bot (Nov 03 2022 at 00:10):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 03 2022 at 00:10):

jameysharp created PR review comment:

My simplest answer is "because I needed ExprVisitor to be a different type from RuleVisitor but I didn't need PatternVisitor to be a different type." In fact for the new IR I'm building that I want to use this in, all three types are the same, because expressions are de-duplicated across if-lets and even across separate rules in the same term.

That's a pretty weak justification though. There's certainly a nice symmetry argument for making both be associated types, and it could support more general usage. It also gives RuleVisitors the opportunity to ignore patterns, like they currently could ignore expressions.

But making PatternVisitor an associated type doesn't change lower_rule at all, or Rule::visit significantly. The fundamental reason those look weird is because visiting a pattern returns nothing, while visiting an expression returns some result. I think that distinction is inherent. PatternIds name the inside of a pattern, top-down, so a PatternVisitor can record anything it might need as it goes. ExprIds name the outside of an expression, bottom-up, so there's a final result that the visitor might need to do something with.

It looks especially weird with the current IR because PatternSequence::add_expr returns ownership of an entire heap-allocated ExprSequence. My new IR just has to return an array index there, so I think that'll be a little less confusing.

Anyway, I've implemented this because the symmetry really is nice. It makes the RuleVisitor trait definition noisy with <Self::PatternVisitor as PatternVisitor>::PatternId but so it goes.

Wasmtime GitHub notifications bot (Nov 03 2022 at 00:16):

jameysharp has enabled auto merge for PR #5174.

Wasmtime GitHub notifications bot (Nov 03 2022 at 01:18):

jameysharp merged PR #5174.

Last updated: Dec 23 2024 at 12:05 UTC