wasmtime / PR #5517 Cranelift: GVN spectre guards and run... · git-wasmtime

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 by other_side_effects(true) shouldn't matter, right?

Wasmtime GitHub notifications bot (Jan 04 2023 at 19:52):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2023 at 19:52):

fitzgen has enabled auto merge for PR #5517.

Wasmtime GitHub notifications bot (Jan 04 2023 at 19:55):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2023 at 19:55):

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 (eg uadd_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.

Wasmtime GitHub notifications bot (Jan 04 2023 at 20:03):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 04 2023 at 20:03):

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?

Wasmtime GitHub notifications bot (Jan 04 2023 at 20:05):

fitzgen merged PR #5517.

Last updated: Nov 22 2024 at 17:03 UTC