Wasmtime GitHub notifications bot (Apr 04 2023 at 18:12): Wasmtime GitHub notifications bot (Apr 04 2023 at 18:12):jameysharp commented on issue #6141:
I like this commit. (Note for other reviewers, this PR is based on #6140 so at the moment only ISLE: simplify select/bitself when both choices are the same is necessary to review here.)
I'd like to double-check with @cfallin that these new rules are correct, but I think they are. I don't think there's any subtlety to
selectorbitselectthat would make this optimization invalid.I think this particular optimization might even be legal for
select_spectre_guard, although that definitely has subtlety and also I wouldn't expect such a rule to ever fire.There's a typo in the commit message and PR description ("bitself"→"bitselect") but once #6140 lands and this PR is rebased, I think this is a fine change.
Wasmtime GitHub notifications bot (Apr 04 2023 at 18:22):cfallin commented on issue #6141:
Yep, this looks right to me -- no hidden subtleties!
I agree that the Spectre-guard variant would also be eligible (and would not lose any speculative safety), but I'm hesitant for us to do that, at least for now; IMHO a better approach for optimizations that touch
select_spectre_guardis probably to (i) make changes that affect it only in concert with updates or optimizations to bounds-checking as a whole (i.e., when looking at generated code and improving it), when we're explicitly thinking about these issues; and (ii) do it in a separate PR, with a detailed argument around its speculative safety, probably also adding documentation on the topic.
Last updated: Nov 22 2024 at 16:03 UTC