-
Notifications
You must be signed in to change notification settings - Fork 562
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#2440 AArch64 XINST_CREATE: Add missing platform-independent macros. #2441
Conversation
However, we omit XINST_CREATE_jump_mem, which is impossible on AArch64. Also remove XINST_CREATE_and, which was added in error, and reorder to match order of other architectures. Change-Id: I9f6ef6cc321b15c49621c79bc7b4de82ee06f8f9
The AppVeyor failure is i#2246. |
core/arch/aarch64/emit_utils.c
Outdated
@@ -690,8 +690,11 @@ emit_indirect_branch_lookup(dcontext_t *dc, generated_code_t *code, byte *pc, | |||
TLS_MASK_SLOT(ibl_code->branch_type), | |||
OPSZ_16))); | |||
/* and x1, x1, x2 */ | |||
APP(&ilist, XINST_CREATE_and(dc, opnd_create_reg(DR_REG_X1), | |||
opnd_create_reg(DR_REG_X2))); | |||
APP(&ilist, INSTR_CREATE_and_shift(dc, opnd_create_reg(DR_REG_X1), |
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 think that AArch64 should follow what we did for ARM: make simpler convenience macros for the common cases that do not require specifying all these extra operands. We have INSTR_CREATE_and() which automagically adds the DR_SHIFT_NONE by 0 and passes to INSTR_CREATE_and_shimm().
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.
Update: looks like this was done for _add and others, it's just not there for _and.
#define XINST_CREATE_and(dc, d, s) \ | ||
INSTR_CREATE_and_shift((dc), (d), (d), (s), \ | ||
OPND_CREATE_INT8(DR_SHIFT_LSL), OPND_CREATE_INT8(0)) | ||
#define INSTR_CREATE_adds(dc, rd, rn, rm_or_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.
Yes, this is what I was referring to up above: so you have it for some instructions, it just needs to be added for INSTR_CREATE_and.
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.
Done.
And use these simpler macros where possible. Change-Id: Ib8cc8d52c73051826fe407a4a871d5d972197567
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
There is an extraneous line in the commit msg.
The Travis failure is i#1892. |
There are no tests added here. #2443 covers adding the types of tests we have on x86, where we would easily include test changes with a patch like this. |
However, we omit XINST_CREATE_jump_mem, which is impossible on AArch64.
Also remove XINST_CREATE_and, which was added in error, and reorder to
match order of other architectures.
Change-Id: I9f6ef6cc321b15c49621c79bc7b4de82ee06f8f9