Wasmtime GitHub notifications bot (Mar 14 2024 at 20:45): Wasmtime GitHub notifications bot (Mar 14 2024 at 20:45):jameysharp opened PR #8139 from jameysharp:optimize-spectre to bytecodealliance:main:
This commit makes two changes to our treatment of
select_spectre_guard.First, stop annotating this instruction as having any side effects. We only care that if its value result is used, then it's computed without branching on the condition input. We don't otherwise care when the value is computed, or if it's computed at all.
Second, introduce some carefully selected ISLE egraph rewrites for this instruction. These particular rewrites are those where we can statically determine which SSA value will be the result of the instruction. Since there is no actual choice involved, there's no way to accidentally introduce a branch on the condition input.
Wasmtime GitHub notifications bot (Mar 14 2024 at 20:45):jameysharp requested fitzgen for a review on PR #8139.
Wasmtime GitHub notifications bot (Mar 14 2024 at 20:45):jameysharp requested wasmtime-compiler-reviewers for a review on PR #8139.
Wasmtime GitHub notifications bot (Mar 14 2024 at 20:46):jameysharp commented on PR #8139:
Perhaps @cfallin should review this? I was talking about it with him yesterday.
Wasmtime GitHub notifications bot (Mar 14 2024 at 21:13):cfallin requested cfallin for a review on PR #8139.
Wasmtime GitHub notifications bot (Mar 14 2024 at 21:13):cfallin commented on PR #8139:
Sure, happy to take a look.
Wasmtime GitHub notifications bot (Mar 14 2024 at 21:23):cfallin submitted PR review:
This rewrite looks good to me; thanks for thinking through and discussing this yesterday!
A few things I think we'll want:
- Could we have an egraph filetest showing each case?
- I think it would be good to document (or update documentation about) the semantics of
select_spectre_guardand the corresponding justification, a little more than the comments here. In particular:
- The instruction definition's description still says the op is guaranteed not to be removed. That's not true anymore (in fact that's the point of this PR!); can we update to say something about not picking the wrong input even under a mis-speculated branch (as you do in the comment in the ISLE rewrites)?
- Let's note in the ISLE rewrites that removing
select_spectre_guard, if semantically correct for the functional result, always meets this speculation condition: by removing an op we are only ever going to remove computation that could be speculated, we're never adding new speculation.- Finally, can we add a comment in around the instruction description noting that the op is pure because our reasoning about speculation safety tells us it's always safe to remove if functionally correct, and safe to merge with another functionally identical op.
Thanks!
Wasmtime GitHub notifications bot (Mar 14 2024 at 21:44):github-actions[bot] commented on PR #8139:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Mar 19 2024 at 05:56):jameysharp updated PR #8139.
Wasmtime GitHub notifications bot (Mar 19 2024 at 05:56):jameysharp requested wasmtime-core-reviewers for a review on PR #8139.
Wasmtime GitHub notifications bot (Mar 19 2024 at 05:58):jameysharp commented on PR #8139:
I think I've addressed all your comments about documentation, Chris, and I'd appreciate feedback on that aspect. I still need to write filetests exercising these new rules, but at least now we can see that this has a positive effect on the typed-funcref tests, as expected.
Wasmtime GitHub notifications bot (Mar 19 2024 at 18:18):cfallin submitted PR review:
New doc comments look great -- thanks!
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:16):jameysharp updated PR #8139.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:16):jameysharp has enabled auto merge for PR #8139.
Wasmtime GitHub notifications bot (Mar 20 2024 at 22:59):jameysharp merged PR #8139.
Last updated: Dec 23 2024 at 13:07 UTC