-
Notifications
You must be signed in to change notification settings - Fork 570
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 ADR and ADRP instructions. #2317
Conversation
Also adds support for instruction operands to ADR and ADRP encoders.
core/arch/aarch64/codec.c
Outdated
uint bits; | ||
if (!opnd_is_rel_addr(opnd)) | ||
if (opnd_is_rel_addr(opnd)) |
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.
style: {} for multi-line body
core/arch/aarch64/codec.c
Outdated
if (opnd_is_rel_addr(opnd)) | ||
offset = (ptr_int_t)opnd_get_addr(opnd) - | ||
(ptr_int_t)((ptr_uint_t)pc >> scale << scale); | ||
else if (opnd.kind == INSTR_kind) |
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.
ditto
core/arch/aarch64/codec.c
Outdated
if (opnd_is_rel_addr(opnd)) | ||
offset = (ptr_int_t)opnd_get_addr(opnd) - | ||
(ptr_int_t)((ptr_uint_t)pc >> scale << scale); | ||
else if (opnd.kind == INSTR_kind) |
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.
Prefer the interface opnd_is_instr()
nit: commit body starting with "Also" is a little odd (normally the title's content is included in the body) |
@@ -334,6 +334,10 @@ | |||
instr_create_1dst_4src((dc), OP_sub, (rd), (rn), (rm_or_imm), (sht), (sha)) | |||
#define INSTR_CREATE_svc(dc, imm) \ | |||
instr_create_0dst_1src((dc), OP_svc, (imm)) | |||
#define INSTR_CREATE_adr(dc, rt, imm) \ |
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.
There's no doxygen header for these. Looking above: there isn't for any of these. Thus these will not show up in the html docs at all.
OK I see above a FIXME for adding them -- but I would suggest including them on any new code. The original suggestion, still in place, is to automatically generate all of these (including the doxygen).
Also adds support for instruction operands to ADR and ADRP encoders.