Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AArch64/SIMD: Add negative decoding tests for invalid sizes #205

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

hanno-becker
Copy link
Contributor

@hanno-becker hanno-becker commented Mar 17, 2025

Various NEON instructions have a 2-bit size parameter but are
only valid for the values {0b00, 0b01, 0b10}. Notably, this
is the case for all SIMD integer multiplication instructions.

For those instructions, simulator_iclasses.ml previously
listed only the valid encodings. This would test that the functional
description of valid encodings was correct, but it would not test
that the architecture and the model agreed on them being invalid.

This commit relaxes the encoding patterns in simulator_iclasses.ml
so that invalid size encodings are considered as well.

A similar change may be possible for immediates required to be
non-zero, but the architecture is not clear on what decoding does
with them: It says SEE(asimdimm) rather than UNDEF. Those
are therefore not touched for now.

@hanno-becker
Copy link
Contributor Author

@jargh Is this what you were looking for? How is conformance testing expected to proceed for invalid encodings?

Various NEON instructions have a 2-bit size parameter but are
only valid for the values {0b00, 0b01, 0b10}. Notably, this
is the case for all SIMD integer multiplication instructions.

For those instructions, `simulator_iclasses.ml` previously
listed only the valid encodings. This would test that the functional
description of valid encodings was correct, but it would not test
that the architecture and the model agreed on them being invalid.

This commit relaxes the encoding patterns in `simulator_iclasses.ml`
so that invalid size encodings are considered as well.

A similar change may be possible for immediates required to be
non-zero, but the architecture is not clear on what decoding does
with them: It says `SEE(asimdimm)` rather than `UNDEF`. Those
are therefore not touched for now.

Signed-off-by: Hanno Becker <[email protected]>
@jargh
Copy link
Contributor

jargh commented Mar 17, 2025

Yes, this is the sort of thing I have in mind. Generally, the instructions understood by the s2n-bignum decoder and executed by the symbolic simulation machinery are expected to be a proper subset of those accepted on the hardware, because many instructions are missing (though more are being added all the time). I suppose in principle we'd like to check all clearly illegal instruction encodings to make sure the decoder/simulator doesn't accept them. In practice, I had the more modest goal of just keeping bit patterns for instruction forms simple and general where possible, considering it a positive rather than negative feature that it executes a sprinkling of illegal cases.

@hanno-becker hanno-becker marked this pull request as ready for review March 17, 2025 07:20
@hanno-becker
Copy link
Contributor Author

@jargh Ok, thank you! Let me know if you'd like any further changes, or feel free of course to just push them atop.

@aqjune-aws aqjune-aws self-requested a review March 21, 2025 16:07
Copy link
Collaborator

@aqjune-aws aqjune-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to rebase your change to the latest version of the main branch? Or I can update this PR as well.

@hanno-becker
Copy link
Contributor Author

Thank you @aqjune-aws. Are we good to merge?

@jargh
Copy link
Contributor

jargh commented Mar 23, 2025

Looks very good to me too - even the comments are right! I'll make a few last simulator runs as a final sanity check and merge later today.

@jargh jargh merged commit 31c3987 into awslabs:main Mar 24, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants