-
Notifications
You must be signed in to change notification settings - Fork 566
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
i#1569 AArch64: Add macro to create BL instructions. #2332
Conversation
core/arch/aarch64/instr_create.h
Outdated
|
||
/** | ||
* This INSTR_CREATE_xxx macro creates an instr_t with opcode OP_xxx and | ||
* the given explicit operands, automatically supplying any implicit operands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two line above seems general and not specific to this new INSTR_CREATE_bl. Kind of strange.
Could you please make it more specific to this instr_create, e.g. creates an branch & link ...
Xref other INSTR_CREATE... with examples.
Or no comment, as the rest ones without comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I picked a doc string from some X86 instruction. But I agree it makes more sense to be specific here. I'll update the comment.
Missing docs for the other macros is an issue that needs to be addressed, but we should try to include proper doc strings for newly added instructions.
Is there an ETA for auto-generating macros for every instruction? If that's soon, these one-off adds will be overwritten. |
Ideally we would like to improve the clean call code generation for AArch64 first, I think the macro for BL should be the last missing macro related to that. For the encoder/decoder: we are investigating how to best add support for the complete ARM v8-a ISA and generating macros for all supported instructions would be part of that. I hope that we have a good idea by the end of the next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this bl instruction break the DR IR's promise, i.e., list all explicit and implicit operands?
core/arch/aarch64/instr_create.h
Outdated
* \param pc The opnd_t target operand containing the program counter to jump to. | ||
*/ | ||
#define INSTR_CREATE_bl(dc, pc) \ | ||
instr_create_0dst_1src((dc), OP_bl, (pc)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't bl have two operands:
one explicit target operand, and one implicit operand link register.
DR IR supposes to list both explicit and implicit operands to make the instruction operand iteration easier.
Otherwise, we have to add a lot of special checking code for implicit operands, e.g., to list all register touched by bl, we have to add code saying link register is also touched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are right! We are missing an implicit X30 destination operand for BL and BLR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the patch to add X30 as implicit dst operand. It requires support by the encoder/decoder though, which I created a separate PR for: #2345
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, I saw the test failure from the list, thought you did not finish it yet. |
Hm |
This patch adds an INSTR_CREATE_bl macro to create branch and link instructions on AArch64.
On any test failure, either file a new issue or cite an existing issue that covers it. |
This patch adds an INSTR_CREATE_bl macro to create branch and link instructions on AArch64.
No description provided.