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

[AMDGPU] Allow lit() on operands which do not accept modifiers #69527

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

rampitec
Copy link
Collaborator

No description provided.

@rampitec rampitec requested review from jayfoad and Sisyph October 18, 2023 21:26
@llvmbot llvmbot added backend:AMDGPU mc Machine (object) code labels Oct 18, 2023
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

This looks OK but I'm not too familiar with operand parsing. Is there a cleaner way to implement this, so that we don't have two different places where "lit" is parsed?

@jayfoad jayfoad requested a review from kosarev October 19, 2023 10:45
@rampitec
Copy link
Collaborator Author

This looks OK but I'm not too familiar with operand parsing. Is there a cleaner way to implement this, so that we don't have two different places where "lit" is parsed?

It boils down to the place where we have to create an operand. That is where I have to set modifiers. I can guess there might be a better way, but this would literally mean rewriting everything, not just parser. I found this a lesser evil.

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

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

This looks OK but I'm not too familiar with operand parsing. Is there a cleaner way to implement this, so that we don't have two different places where "lit" is parsed?

It boils down to the place where we have to create an operand. That is where I have to set modifiers. I can guess there might be a better way, but this would literally mean rewriting everything, not just parser. I found this a lesser evil.

LGTM then. We can always rewrite it later...

@rampitec rampitec merged commit f7ab79f into llvm:main Oct 19, 2023
4 checks passed
@rampitec rampitec deleted the lit-modifier branch October 19, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants