fitzgen requested alexcrichton for a review on PR #3556.
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.
-->
fitzgen requested cfallin for a review on PR #3556.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
s/doesn't/don't/
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 :-)
cfallin created PR review comment:
This paragraph has good detail but is a little dense; a snippet or two showing how
sinkable_load
andsink_load
are used in a rule might help?
jlb6740 submitted PR review.
jlb6740 submitted PR review.
jlb6740 created PR review comment:
s/fnally/finally/
jlb6740 created PR review comment:
s/themselves they//
jlb6740 submitted PR review.
jlb6740 submitted PR review.
jlb6740 created PR review comment:
? caare
jlb6740 submitted PR review.
jlb6740 submitted PR review.
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.
jlb6740 edited PR review comment.
jlb6740 edited PR review comment.
alexcrichton submitted PR review.
avanhatt submitted PR review.
avanhatt created PR review comment:
Is this/can this be enforced by the ISLE compiler? Or is it the responsibility of the rule-writer?
cfallin submitted PR review.
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 intoInputContext
andOutputContext
, and hold only an immutable borrow toInputContext
, which would give us some type-level enforcement of this, at least...
fitzgen submitted PR review.
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.
fitzgen submitted PR review.
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)
fitzgen updated PR #3556 from isle-integration-docs
to main
.
fitzgen submitted PR review.
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.
fitzgen updated PR #3556 from isle-integration-docs
to main
.
cfallin submitted PR review.
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" :-)
fitzgen merged PR #3556.
fitzgen submitted PR review.
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.
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!
cfallin submitted PR review.
Last updated: Dec 23 2024 at 12:05 UTC