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

Added support for multiple OperandConstraint::Reuse operands. #180

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Iizerd
Copy link

@Iizerd Iizerd commented Jul 25, 2024

Previously instructions that have multiple operands specifying the constraint OperandConstraint::Reuse would not work properly. Good examples of this are X86's XCHG and XADD, both of which require two OperandConstraint::Reuses. This PR addresses this issue and fixes it by using a SmallVec<[VReg; 4]> to store all reuse constraints to check, instead of a single Option<VReg>.

I ran 8 instances of ion_checker overnight without fail. Additionally, the fixes have enabled my compiler to correctly emit the aforementioned instructions and it passes my own test suite. I'm not however confident that the ion_checker correctly generates these types of instructions as prior to this change it was still reporting no errors. It also seems to only generate one Def per instruction.

I will be leaving a comment in #145 as I discovered some interesting things about that as well while looking into this.

As discussed in the other PR(sorry I had to recreate to change source branch) I will look into changing the fuzzer, I will hopefully find time to do this within the coming week or two.

@cfallin
Copy link
Member

cfallin commented Sep 3, 2024

Hi @Iizerd -- I'd still like to see the fuzzing for this before we merge it, to be sure we've got it right -- any update on this?

As discussed in the other PR(sorry I had to recreate to change source branch) I will look into changing the fuzzer, I will hopefully find time to do this within the coming week or two.

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