Wasmtime GitHub notifications bot (Mar 17 2023 at 17:02): Wasmtime GitHub notifications bot (Mar 17 2023 at 17:02):elliottt opened PR #6047 from trevor/validate-opcode-signatures to main:
Note: this PR is rebased on top of #6046, and shouldn't be merged without it.
Add a program to cranelift-fuzzgen that can be used to validate the
OPCODE_SIGNATUREStable that we maintain in cranelift-fuzzgen. Additionally, fix some signatures that were identified as not matching the signature from the instruction's specification.This is incremental work towards generating instruction instantiations in cranelift-fuzzgen using the instruction specification instead of explicitly opting-in an instruction.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:03):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:03):elliottt created PR review comment:
An alternate approach here would be to instead use the signature to check the
OPCODE_SIGNATUREStable, however explicitly generating all instantiations leaves the door open to using that set to generate the complement of signatures that we don't currently generate instructions for when fuzzing.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:05):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:05):elliottt created PR review comment:
One question I have here is if this is the best way to report this information. Perhaps reworking this program as a test instead would be better, as that would prevent any backsliding on the existing signatures?
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:07):elliottt has marked PR #6047 as ready for review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:07):elliottt requested afonso360 for a review on PR #6047.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:15):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:22):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:54):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:54):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:54):afonso360 created PR review comment:
I was initially surprised as to how this didn't crash, but I guess we never generate
F64X4's so we never selected this!
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:54):afonso360 created PR review comment:
This took me a while to figure out, but I think its the same thing I wrote in my proof of concept code a while ago!
Adding some sort of comment explaining this operation at a high level would be really helpful.
In my POC I borrowed the name from Itertools::multi_cartesian_product which does the same operation, I'm not sure if that is the correct mathematical name for the operation, but it made sense at the time.
Wasmtime GitHub notifications bot (Mar 17 2023 at 17:54):afonso360 created PR review comment:
Yeah, changing this to be a test would be awesome, that way we definitely wouldn't miss wrong signatures. (which there are way more than I would have thought :sweat_smile:)
Wasmtime GitHub notifications bot (Mar 17 2023 at 18:21):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 18:21):elliottt created PR review comment:
That's definitely the right name -- I'll add some comments so this thing isn't quite so opaque :)
Wasmtime GitHub notifications bot (Mar 17 2023 at 18:24):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:02):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:02):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:03):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:03):elliottt created PR review comment:
I reworked it as a test, so you'll see the output if a new signature is added that doesn't match the constraints of the instruction DSL :+1:
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:08):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:08):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:09):elliottt edited PR #6047 from trevor/validate-opcode-signatures to main:
Note: this PR is rebased on top of #6046, and shouldn't be merged without it.
Add a test to cranelift-fuzzgen that validates the
OPCODE_SIGNATUREStable by comparing it against valid instruction instantiations. Additionally, fix some signatures that were identified as not matching the signature from the instruction's specification.This is incremental work towards generating instruction instantiations in cranelift-fuzzgen using the instruction specification instead of explicitly opting-in an instruction.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:11):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:11):elliottt created PR review comment:
This is the current list of discrepancies that we'll need to fix to ensure that
OPCODE_SIGNATURESis reflecting the current state of instruction constraints. A separate issue will be to reconcile the instruction constraints with the current behavior of the backends, but we'll get there eventually :)
Wasmtime GitHub notifications bot (Mar 17 2023 at 21:40):elliottt edited PR #6047 from trevor/validate-opcode-signatures to main:
Add a test to cranelift-fuzzgen that validates the
OPCODE_SIGNATUREStable by comparing it against valid instruction instantiations. Additionally, fix some signatures that were identified as not matching the signature from the instruction's specification.This is incremental work towards generating instruction instantiations in cranelift-fuzzgen using the instruction specification instead of explicitly opting-in an instruction.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
Wasmtime GitHub notifications bot (Mar 17 2023 at 23:21):elliottt updated PR #6047 from trevor/validate-opcode-signatures to main.
Wasmtime GitHub notifications bot (Mar 18 2023 at 19:14):elliottt merged PR #6047.
Last updated: Dec 23 2024 at 12:05 UTC