[feedback] per-edge dataflow #47664
[feedback] per-edge dataflow #47664
Conversation
It used to take an `in_out` but that made it hard to determine whether the call had any effect or not.
Don't use for calls yet.
No functional change intended.
The "access tracker" should seamlessly distiguish: - No edge effects at all -- in this case there would be no assignments at all, and hence the "maybe mutated" flag should be constant propagated to false. - Sometimes edge effects -- in this case, we can determine if in fact a gen or kill bit was added in an O(1) check, rather than the old O(n) check.
|
cc @eddyb, who's been looking at this code lately |
|
(I'd probably prefer to land the |
carols10cents
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #47837) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@pnkfelix and @rust-lang/compiler, ping from triage! |
kennytm
added
the
T-compiler
|
@nikomatsakis this PR needs a rebase! |
|
I think I will just close it, seeing as how it's just cleanup, and not particularly vital cleanup at that. Maybe I'll rebase someday if we ever care. |
| @@ -542,7 +542,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for MovingOutStatements<'a, 'gcx, 'tcx> { | |||
| } | |||
|
|
|||
| fn propagate_call_return(&self, | |||
| in_out: &mut IdxSet<MoveOutIndex>, | |||
| sets: &mut BlockSets<'_, MoveOutIndex>, | |||
There was a problem hiding this comment.
This comment may be made irrelevant by later commits in the series, but I don't think it was correct to replace IdxSet with BlockSets here.
IIRC, BlockSets is a triple of sets associated with the block currently being processed. But fn propagate_call_return needs to modify the successor block for the call terminator, which is why we pass in the whole in_out here and not just the three sets for the current block.
There was a problem hiding this comment.
(I think a commit further down the line did indeed make this feedback irrelevant. If it was ever relevant in the first place, which I am no longer certain of; the IdxSet here is not for the whole universe of blocks anyway...)
| /// When its false, no local clone is constucted; instead it is | ||
| /// undefined what `on_entry` points to (in practice, it will | ||
| /// frequently be a reference the flow state at the block's start, | ||
| /// but you should not rely on that). |
There was a problem hiding this comment.
I think this is a good change (in that no one should be or have been using the stronger guarantee in the earlier version of the comment). But I am curious: Did you actually concoct some scenario that invalidated the comment's stronger claim? Or are you figuring that we're better off not defining the semantics in this case?
|
Oh whoops, forgot to reload this page before I started reviewing this. Okay well if the PR's closed then I'll stop reviewing. |
|
@pnkfelix well depends on whether you liked it =) That said, in answer to your question:
I forget, but I think there was a counterexample. |
This is a branch I had lying around. It is an attempt to modify the dataflow APIs to support "bits per edge" -- this was intended to fix a shortcoming in NLL, but then @arielb1 had a better fix anyhow. I'm not sure if this code (or some variant of it) is worth landing or not.
The basic idea is that the data flow operator may define the bits on any given edge, instead of just the "call return edge". The propagation code is rewritten so apply and unroll these effects as it goes; it is optimized for the case that individual edges do not have distinct effects. To micro-optimize, I added what is either a brilliant hack or a terrible idea: it's a wrapper whose
DerefMutimpl sets a flag, so that code which never winds up trying to mutate will be seamlessly detected.Anyway, I have a feeling that these changes are starting to get into "over-designed" territory, so I thought I'd throw them out for some feedback.
r? @pnkfelix