Cranelift: implement general select_spectre_guard fallbacks. #5420
Conversation
When adding some optimization rules for `icmp` in the egraph infrastructure, we ended up creating a path to legal CLIF but with patterns unsupported by three of our four backends: specifically, `select_spectre_guard` with a general truthy input, rather than an `icmp`. In bytecodealliance#5206 we discussed replacing `select_spectre_guard` with something more specific, and that could still be a long-term solution here, but doing so now would interfere with ongoing refactoring of heap access lowering, so I've opted not to do so. (In that issue I was concerned about complexity and didn't see the need but with this fuzzbug I'm starting to feel a bit differently; maybe we should remove this non-orthogonal op in the long run.) Fixes bytecodealliance#5417.
github-actions
bot
added
cranelift
There was a problem hiding this comment.
I'm glad to see select_spectre_guard implemented for the general case. I think what I'm seeing here is that you're choosing the most general, but not necessarily most efficient, implementation of select from each backend and copying that as the fallback for this instruction? That makes sense to me, at least for now. But I don't feel comfortable reviewing the actual implementations.
Maybe what we should learn from this particular fuzz-bug is that if we have any combination of CLIF instructions that we can't lower, we will eventually generate that combination, probably by accident. Given that, perhaps we should adopt the principle/goal that all combinations of CLIF which pass the validator should be implemented in any tier 1 backend? Whenever we fail that, it means we need to either change the instruction set, change the validator, or change the backend.
|
Yes, exactly, I took the fallback rules for
That sounds like a reasonable principle to me! |
|
Ah I thought that Jamey already reviewed it so I didn't leave a comment. Unclear to me whether Jamey wants changes on this PR after their review or not? |
|
I interpreted @jameysharp 's reply as: no changes needed, approach is fine, but:
FWIW, the implementations are identical to the existing |
This PR follows up on #5382 and #5391, which rebuilt the egraph-based optimization framework to be more performant, by enabling it by default. Based on performance results in #5382 (my measurements on SpiderMonkey and bjorn3's independent confirmation with cg_clif), it seems that this is reasonable to enable. Now that we have been fuzzing compiler configurations with egraph opts (#5388) for 6 weeks, having fixed a few fuzzbugs that came up (#5409, #5420, #5438) and subsequently received no further reports from OSS-Fuzz, I believe it is stable enough to rely on. This PR enables `use_egraphs`, and also normalizes its meaning: previously it forced optimization (it basically meant "turn on the egraph optimization machinery"), now it runs egraph opts if the opt level indicates (it means "use egraphs to optimize if we are going to optimize"). The conditionals in the top-level pass driver are a little subtle, but will get simpler once we can remove the non-egraph path (which we plan to do eventually!). Fixes #5181.
When adding some optimization rules for
icmpin the egraph infrastructure, we ended up creating a path to legal CLIF but with patterns unsupported by three of our four backends: specifically,select_spectre_guardwith a general truthy input, rather than anicmp.In #5206 we discussed replacing
select_spectre_guardwith something more specific, and that could still be a long-term solution here, but doing so now would interfere with ongoing refactoring of heap access lowering, so I've opted not to do so. (In that issue I was concerned about complexity and didn't see the need but with this fuzzbug I'm starting to feel a bit differently; maybe we should remove this non-orthogonal op in the long run.)Fixes #5417.