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. #179

Closed
wants to merge 3 commits into from

Conversation

Iizerd
Copy link

@Iizerd Iizerd commented Jul 19, 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.

@cfallin
Copy link
Member

cfallin commented Jul 19, 2024

Thanks for looking into this! The change does look correct to me.

Regarding fuzzing -- I believe you're correct that we actually only add one def per instruction and up to one reuse of that def when generating test functions (see here). It would be good to see if we can relax that restriction (on both counts) and test with that.

@Iizerd Iizerd closed this Jul 24, 2024
@cfallin
Copy link
Member

cfallin commented Jul 25, 2024

@Iizerd any particular reason you've closed this PR? We're happy to take the change, with the fuzzing changes as suggested above!

@Iizerd
Copy link
Author

Iizerd commented Jul 25, 2024

I realized I should have created a separate branch with just the one commit that I made relating to that. I did not realize that future commits(as seen above) would also get added to the pr 👀 whoops! I can reopen it but let me create a separate branch that only has the one commit first that actually makes the change mentioned.

@Iizerd Iizerd reopened this Jul 25, 2024
@Iizerd Iizerd closed this Jul 25, 2024
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