iterate_to_fixpoint -> iterate_reachable_to_fixpoint

[smoelius]

The rustc_mir_dataflow::framework::Engine computes a fixpoint over just the reachable basic blocks instead of all of them, and does not offer a way to do the latter. This PR addresses that problem.

Specifically, this PR does the following:

• Renames iterate_to_fixpoint to iterate_reachable_to_fixpoint.
• Adds to framework::Enginge a new method with the old name (iterate_to_fixpoint). The new method computes the fixpoint over all basic blocks, instead of just the reachable ones.
• Changes one occurrence of iterate_to_fixpoint to iterate_reachable_to_fixpoint where the analysis seemed to care about only reachable basic blocks.

For context, I asked this question in the Rust language forum. Then I noticed that framework::Results has both visit_with and visit_reachable_with methods.

Arguably, the current API is inconsistent (and perhaps even dangerous). Why would someone visit all basic blocks when iterate_to_fixpoint computes the fixpoint over just the reachable ones? It seems unlikely that someone would do this intentionally.

Currently, there is just one call to visit_reachable_with in the compiler directory. That one call is related to a use of MaybeRequiresStorage. This PR changes its associated call to iterate_to_fixpoint to iterate_reachable_to_fixpoint, since that analysis seemed to really want just reachable basic blocks.

Presumably, all calls to visit_with are either agnostic to whether they visit all or only reachable basic blocks, or really want all basic blocks. This PR effectively changes those calls to use the new method with the old name.

[rust-highfive]

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

I'm not sure who is responsible for this code.
r? rust-lang/compiler

[smoelius]

I am taking the liberty of cc-ing @ecstatic-morse, who according to git blame, is responsible for inserting the call to visit_reachable_with.

[jackh726]

Let's r? @pnkfelix, since they reviewed #74169 where this code was seemingly introduced.

[ecstatic-morse]

I don't think this is desirable. It's always advantageous to prune unreachable basic blocks before running a dataflow analysis. Dataflow is not meaningful for unreachable blocks. We should be pushing users towards that instead of adding more API surface. visit_reachable_with was intended as a stopgap to keep the generator transform working. It's not something to aspire to.

I'm happy to explain the changes that would be required in the generator transform to make this a reality.

[ecstatic-morse]

The order in which blocks are added to the work queue is significant. I suspect this will hurt performance. It would be kind of fun to check though, since the basic block numbering is usually fairly close to RPO due to how we build MIR.

[smoelius]

@ecstatic-morse

Please allow me to add some more context.

In my analysis, I am identifying SwitchInt terminators for which the scrutinee is a Result and pruning their Ok edges, because I only care what happens along error paths. So the unreachable blocks are not unreachable in an absolute sense, but only after pruning.

The analysis works beautifully with the new iterate_to_fixpoint. It doesn't work with iterate_reachable_to_fixpoint because the (after pruning) unreachable blocks are incorrectly left at the bottom value.

I understand the desire to not introduce additional API surface. But in that case, I feel a workaround ought to exist for folks like me. As best I can tell, if this PR is not merged, then the only alternative I have is to re-implement iterate_to_fixpoint within my own code. Am I wrong?

[ecstatic-morse]

And to do this you're deleting edges in the CFG? You should use switch_int_edge_effects instead.

[smoelius]

And to do this you're deleting edges in the CFG?

Yes, using MirPatch.

You should use switch_int_edge_effects instead.

That won't work because my analysis is backward, and switch_int_edge_effects is not used in backward analyses:

rust/compiler/rustc_mir_dataflow/src/framework/mod.rs

Line 238 in 8b67c5e

/// FIXME: This class of effects is not supported for backward dataflow analyses.

This gives another way of looking at the problem. "Reachable" here means reachable from the start block. So by computing fixpoints over just "reachable" blocks, iterate_to_fixpoint demonstrates a bias toward forward analyses.

If you don't want to introduce additional API surface, then I think another way to fix this problem may be to change how the queue is primed for backward analyses. Essentially, replace this code:

rust/compiler/rustc_mir_dataflow/src/framework/engine.rs

Lines 214 to 218 in 8b67c5e

	// Reverse post-order on the reverse CFG may generate a better iteration order for
	// backward dataflow analyses, but probably not enough to matter.
	for (bb, _) in traversal::postorder(self.body) {
	dirty_queue.insert(bb);
	}

with this code:

rust/compiler/rustc_mir_dataflow/src/framework/engine.rs

Lines 193 to 195 in 8b67c5e

	self.body.basic_blocks().indices().for_each(|bb| {
	dirty_queue.insert(bb);
	});

[ecstatic-morse]

That won't work because my analysis is backward, and switch_int_edge_effects is not used in backward analyses.

This is not a fundamental limitation. It's just a FIXME that no one ever got around to fixing. Unfortunately we don't keep any reverse-lookup data structures for SwitchInt terminators, so it will be a bit slow unless you add one.

Another way to do what you want is to extend dead_unwinds to ignore arbitrary edges. dead_unwinds does basically the same thing you're doing now, but it only works for a single edge type.

This gives another way of looking at the problem. "Reachable" here means reachable from the start block. So by computing fixpoints over just "reachable" blocks, iterate_to_fixpoint demonstrates a bias toward forward analyses.

For better or worse, computer programs execute forwards, so reachability remains an important criterion.

If you don't want to introduce additional API surface, then I think another way to fix this problem may be to change how the queue is primed for backward analyses. Essentially, replace this code:

Once again, the initial ordering of basic blocks in the work queue is important. Choosing a good one reduces the number of iterations it takes to reach fixpoint. Can you see why this is the case? That's why the code added unreachable ones at the end prior to #74169.

[smoelius]

For better or worse, computer programs execute forwards, so reachability remains an important criterion.

Right, but the bottom line is: there are backward analyses that the current framework cannot handle. I have run into one; there are likely to be others. We could address my particular case by implementing apply_switch_int_edge_effects or extending dead_unwinds, but that would just be a band-aid. If I may be frank, priming the work queue with forward-reachable blocks for a backward analysis just doesn't make sense.

Anyway, I feel as though I cannot convince you of my point of view. So, of the two suggestions you proposed, implementing apply_switch_int_edge_effects seems slightly less hacky. I will submit a separate PR for that and request you as a reviewer, ok?

Finally, just to answer your question:

Once again, the initial ordering of basic blocks in the work queue is important. Choosing a good one reduces the number of iterations it takes to reach fixpoint. Can you see why this is the case?

Yes, but I think the order is a red herring. My point was about changing the work queue's contents, not its order.

[smoelius]

@ecstatic-morse

Thank you for reviewing #95120. I'm closing this PR because #95120 provides me a workable solution. But please continue to evaluate your position on this:

priming the work queue with forward-reachable blocks for a backward analysis

Best.