Stream: git-wasmtime

Topic: wasmtime / PR #3556 Add a document describing how ISLE is...

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:22):

fitzgen requested alexcrichton for a review on PR #3556.

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:22):

fitzgen opened PR #3556 from isle-integration-docs to main:

<!--

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 19 2021 at 22:22):

fitzgen requested cfallin for a review on PR #3556.

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:45):

cfallin created PR review comment:

s/doesn't/don't/

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:45):

cfallin created PR review comment:

This SinkableLoad discussion could refer to the section below as well, noting that it's a form of type safety, disallowing use of the value until we commit to merging it :-)

Wasmtime GitHub notifications bot (Nov 19 2021 at 22:45):

cfallin created PR review comment:

This paragraph has good detail but is a little dense; a snippet or two showing how sinkable_load and sink_load are used in a rule might help?

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:37):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:37):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:37):

jlb6740 created PR review comment:

s/fnally/finally/

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:37):

jlb6740 created PR review comment:

s/themselves they//

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:49):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:49):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:49):

jlb6740 created PR review comment:

? caare

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:57):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:57):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:57):

jlb6740 created PR review comment:

The description of what constitutes a side effect, I am having a hard time appreciating what's really being described or taking it an applying it and thinking up a different example. Is there some way or mechanism to help prevent writing code that violates this no side effect rule? With the description of a side effect here it worries me that it will be easier to create bugs that are harder to track down.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:57):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Nov 19 2021 at 23:58):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (Nov 22 2021 at 16:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2021 at 22:20):

avanhatt submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2021 at 22:20):

avanhatt created PR review comment:

Is this/can this be enforced by the ISLE compiler? Or is it the responsibility of the rule-writer?

Wasmtime GitHub notifications bot (Nov 23 2021 at 22:31):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2021 at 22:31):

cfallin created PR review comment:

The latter; the external extractor is a call into Rust, which could in theory do anything.

I had suggested at one point (in a conversation with @fitzgen I think) that we could split the Context trait into InputContext and OutputContext, and hold only an immutable borrow to InputContext, which would give us some type-level enforcement of this, at least...

Wasmtime GitHub notifications bot (Nov 29 2021 at 19:36):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 19:36):

fitzgen created PR review comment:

I thought some more about this and determined that if we actually did this, it would make rewriting our peephole optimizations in ISLE impossible because both the input and output contexts would need to borrow the DFG (shared/immutable borrow for input, exclusive/mutable for output) which would be a borrowck error. Or at least it would force us to use RefCell pervasively, which could be a perf hit and certainly would be a pain to work with.

Wasmtime GitHub notifications bot (Nov 29 2021 at 19:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2021 at 19:37):

fitzgen created PR review comment:

(Even the existing lowering doesn't have an input and output LowerCtx so we would have the same issues with instruction selection, even though we "shouldn't" if we were building this all from scratch)

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:13):

fitzgen updated PR #3556 from isle-integration-docs to main.

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:24):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 00:24):

fitzgen created PR review comment:

I rewrote this paragraph a bit and broke it up with examples, as Chris suggested. Hope it is clearer now.

It has me thinking, is there some way or mechanism to help prevent writing code that violates this no side effect rule? With the description of a side effect here it worries me that it will be easier to create bugs that are harder to track down.

It's the same thing you have to deal with writing Rust code right now too. If you do

if something_with_side_effects() {
    foo();
} else {
    bar();
}

the side effects of the condition aren't rolled back if you take the bar(); branch.

Unfortunately, this isn't really something we can reason about in ISLE itself because all the things that could perform side effects are external Rust helpers.

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:02):

fitzgen updated PR #3556 from isle-integration-docs to main.

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:20):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 01:20):

cfallin created PR review comment:

@fitzgen I think that the peephole (CLIF-to-CLIF) case is a separate design question with some interesting possibilities. The most natural approach (given our constraints) might actually be to send emitted instructions to a separate buffer (either a patch buffer on top of the IR with a later batch-update, or just emit new IR in a straight-through pass), in which case the Input and Output contexts are truly independent. That has the advantage of making the lowerings fine-grained-parallelizable as well. And it would allow us to move to a more VCode-like representation of "compact array of insts, no redirections or next/prev links" which is much more efficient to stream through. The downside is of course that it might take multiple passes to converge, but that's an effort-vs-quality tradeoff knob we can turn.

Anyway, that's a separate discussion (and one I'd love to dive into more!) but the relevance here is just to say "there's another way" :-)

Wasmtime GitHub notifications bot (Nov 30 2021 at 03:16):

fitzgen merged PR #3556.

Wasmtime GitHub notifications bot (Nov 30 2021 at 17:43):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 17:43):

fitzgen created PR review comment:

That would make it possible, yes. But it's also a lot further off than "porting simple preopt to ISLE" is, at least in my mind.

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:22):

cfallin created PR review comment:

Hmm, interesting -- we should definitely talk design more here, I think. I'm somewhat uncomfortable with the idea of a pass that does in-place edits, or at least it's not what I had in mind when designing the DSL or when thinking about the pipeline-of-streaming-passes architecture that the new backends are built around. My main concern is that it would paint us into a corner (in terms of rules that we write) where we depend on the immediate availability of the result of an edit, and can't get out of this corner later when we want to do any sort of batch-update optimization (separate streaming passes, or parallelization, or anything like that).

I think that it actually may not be so bad to buffer new instructions at insertion points and "replace value X with value Y" actions. In my estimation probably not any more work than what it would take to build in-place editing glue; both seem like ~a few hundred lines of code, in my estimation.

Anyway, we've branched off the thread topic here, and this is the sort of architectural thing that probably merits at least an issue thread of discussion, so I'm happy to discuss more elsewhere when it comes time for this!

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:22):

cfallin submitted PR review.

Last updated: Oct 23 2024 at 20:03 UTC