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

x86: Fix semantics for pinsrw, add semantics for pinsr{b,d,q} #183

Merged
merged 3 commits into from
Dec 22, 2020

Conversation

chameco
Copy link
Contributor

@chameco chameco commented Dec 17, 2020

Fixes are as follows:

  • Treat the offset as relative to the end of the destination bitvector instead of the beginning. (A FIXME comment mentioned that this might need to be changed, and some testing corroborated this.)
  • Only use some instruction-specific number of low bits from the immediate operand. This was resulting in different behavior when the immediate was "too big", for example something like pinsrd xmm1, eax, 5 should result in an offset of 5 & 0b11 = 1.

@chameco chameco marked this pull request as ready for review December 21, 2020 17:37
Comment on lines 2454 to 2455
=> NatRepr w
-> NatRepr c
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get a comment explaining what w and c are here?

Are the mask values just taken from the ISA manual?

Copy link
Contributor Author

@chameco chameco Dec 22, 2020

Choose a reason for hiding this comment

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

I've added some Haddock comments to those parameters.

The ISA manual specifies to use the 1, 2, 3, or 4 least significant bits of the immediate (depending on the instruction / whether the destination is MMX or XMM for pinsrw). I can add comments explaining the masks in this light if you'd like. I could also use a binary literal which makes the masks more apparent, but this would require enabling the BinaryLiterals extension.

Copy link
Contributor

@travitch travitch left a comment

Choose a reason for hiding this comment

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

Thanks, that looks good to me.

@chameco chameco merged commit 2bd0633 into master Dec 22, 2020
brianhuffman pushed a commit to GaloisInc/saw-script that referenced this pull request Jan 7, 2021
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.

2 participants