cranelift: Enable "chaos mode" in egraph pass #7968
Conversation
First of all, thread a "chaos mode" control-plane into Context::optimize and from there into EgraphPass, OptimizeCtx, and Elaborator. In this commit we use the control-plane to change the following behaviors in ways which shouldn't cause incorrect results: - Dominator-tree block traversal order for both the rule application and elaboration passes - Order of evaluating optimization alternatives from `simplify` - Choose worst values instead of best in each eclass Co-authored-by: L. Pereira <lpereira@fastly.com>
jameysharp
requested review from
alexcrichton
and removed request for
a team
|
I don't think I'm qualified to review this (deferring to @cfallin tagged here as well), but nevertheless I wanted to ask a question about this as well:
From the discussion in the recent egraph issue I thought we settled on the opposite conclusion, that the minimum value must be chosen. I suspect though you've already considered that and it's already accounted for here, leading me to my question. I suspect my understanding of the outcome of that discussion is probably no longer accurate, so is there a tl;dr; or why that issue won't resurface if we choose any value within an eclass? (if there's not a tl;dr; that's also totally ok, happy to go hunting elsewhere too!) |
| @@ -248,7 +269,17 @@ impl<'a> Elaborator<'a> { | |||
| // is a `(cost, value)` tuple; `cost` comes first so | |||
| // the natural comparison works based on cost, and | |||
| // breaks ties based on value number. | |||
| best[value] = std::cmp::min(best[x], best[y]); | |||
| best[value] = if use_worst { | |||
There was a problem hiding this comment.
I wonder if there' a way to encapsulate this? I realize it's a bit tricky because the order relation actually changes based on use_worst (reserved values come "last" when picking mins, and "first" when picking maxes). Maybe something like pick_non_reserved_with_cmp(|x, y| x <= y) or something like that, then picked_non_reserved_with_cmp takes two (Cost, Value) tuples and has the if-ladder?
Maybe that's just the same complexity as before, I dunno; just code-golfing a bit to see if we can make it prettier :-)
There was a problem hiding this comment.
We discussed this briefly while writing, and decided on this as it's more explicit and it's used only here.
|
Yeah, that's absolutely a fair question, @alexcrichton. In ongoing discussion we're currently thinking that we need to establish an invariant that it is always correct to choose any value from an eclass. We should only use the cost function to indicate a preference for some values over others, not to encode correctness constraints. So this PR is part of checking that we've done that correctly. I believe that with #7879 we currently have this invariant, but it's currently fragile because it relies on rewrite-rule authors strictly following new guidelines about using |
|
I've been running the cranelift-fuzzgen on this branch locally for over an hour without finding any failures, so that's a good start. Also I forced |
First of all, thread a "chaos mode" control-plane into Context::optimize and from there into EgraphPass, OptimizeCtx, and Elaborator.
In this commit we use the control-plane to change the following behaviors in ways which shouldn't cause incorrect results:
simplify@lpereira and I wrote this together.