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

Rework field and u32 comparison instructions #1380

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Jul 3, 2024

This small PR implements emulation of immediate values for field and u32 comparison instructions:

  • U32Lt, U32Lte, U32Gt, U32Gte, U32Min, U32Max,
  • Lt, Lte, Gt, Gte).

Instead of using additional *Imm(v) instruction variation, they are emulated with [Push(v), *instr*] array.

Related issue: #1374

@Fumuran Fumuran linked an issue Jul 3, 2024 that may be closed by this pull request
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I defer to @bitwalker to review the .lalrpop file.

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

LGTM!

Just one note for the future, for all of us: while this PR does simplify some parts of the assembler, which benefits us internally - the syntax tree data structures (Instruction in this instance) is part of our public API (one which the compiler uses, at a minimum). By removing conveniences from the AST and only providing them via the parser, it does impose additional burden on downstream producers of MASM ASTs to replicate whatever our parser is doing.

In this particular case, it's not really a problem, but I want to note the issue more generally, since I think it is a very easy at the moment to forget that the MASM parser is not the only way to produce MASM AST, and we could end up removing something that has actual usage/value beyond the assembler itself.

@Fumuran
Copy link
Contributor Author

Fumuran commented Jul 10, 2024

Right, I forgot about it honestly. Thank you @bitwalker for the admonition! Probably all subsequent assembler issues first should be discussed with you to make sure that changes in parser/AST won't create any problems for the compiler (or at least these problems could be easily fixed).

@Fumuran Fumuran merged commit 63170ca into next Jul 10, 2024
15 checks passed
@Fumuran Fumuran deleted the andrew-rework-comp-instrs branch July 10, 2024 14:35
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.

Rework field and u32 comparison instructions
3 participants