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

x64: Compare source arguments in produces_const #4709

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Aug 15, 2022

This PR changes the behavior of produces_const in the x64 backend to check that the two source registers are the same, rather than check that the second source and destination are the same. The reason for this change is a mismatch in behavior between the ISLE constructors for those pseudo instructions and the existing rust functions for constructing them: the rust functions always ensure that the first source and destination registers are the same, while the isle functions introduce a fresh destination register and take the two sources as arguments.

This change preserves the existing behavior for instructions constructed through the rust functions, but allows the lower.isle to use instructions like (x64_xor x x) where x is a fresh register without causing a panic in RA2.

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks!

To clarify the impact of this to anyone reading: there is no miscompilation currently, because all producers of these instruction variants set src1 == dst. But it becomes an issue once one starts to produce instructions where this is not true (i.e., actually leveraging the ability to generate SSA-like VCode), which @elliottt's in-progress work is doing. It's still clearly a bug so this fix makes sense on its own :-)

@elliottt elliottt marked this pull request as ready for review August 15, 2022 18:01
@elliottt elliottt enabled auto-merge (squash) August 15, 2022 18:51
@elliottt elliottt merged commit 1d0f6fa into bytecodealliance:main Aug 15, 2022
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