fitzgen opened PR #5517 from gvn-spectre-guards-and-run-redundant-load-elimination-twice
to main
:
These two changes are collectively required to deduplicate two identical Wasm loads with dynamic memories.
fitzgen requested elliottt for a review on PR #5517.
bjorn3 created PR review comment:
nit: missing trailing newline
bjorn3 submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
This is the
CRANELIFT_TEST_BLESS=1
update code, can fix it in a follow up PR.
elliottt submitted PR review.
elliottt submitted PR review.
elliottt created PR review comment:
:tada:
bjorn3 created PR review comment:
Do we actually need
select_spectre_guard
to have "side effects"? The important part is that it lowers to a conditional move whose output can't be speculated, right? Moving, duplicating, merging or removing it (as prevented byother_side_effects(true)
shouldn't matter, right?
bjorn3 submitted PR review.
fitzgen has enabled auto merge for PR #5517.
fitzgen submitted PR review.
fitzgen created PR review comment:
I had this discussion with @cfallin (out of office right now) and the conclusion was that we probably didn't technically need it, but leaving it as-is was a nice way to be conservative here, since we want to be really careful with it.
I do plan to use
side_effects_okay_for_gvn
for some more stuff (eguadd_overflow_trap
) that is not movable (ie invalid to LICM hoist) but is GVN-able, so it isn't like this infrastructure is completely unnecessary.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Maybe call it side_effects_idempotent and add a comment that it allows merging instructions into an identical instruction earlier in the runtime program order but doesn't allow moving?
fitzgen merged PR #5517.
Last updated: Dec 23 2024 at 12:05 UTC