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.

Sign up for GitHub

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

Jump to bottom

i#2440: Macros for creating conditional instructions #4500

Closed
wants to merge 11 commits into from
Closed

i#2440: Macros for creating conditional instructions #4500

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
14bfb4b
i#2440: Macros for creating conditional instructions
yury-khrustalev Oct 23, 2020
08ce2c4
i#2440: Fix formatting issues
yury-khrustalev Oct 29, 2020
3c13a13
i#2440: Fix more formatting issues
yury-khrustalev Oct 29, 2020
16f30ba
i#2440: Fix more formatting issues
yury-khrustalev Oct 29, 2020
4d8f379
i#1569 AArch64: Enable -steal_reg_at_reset for AArch64 (#4503)
derekbruening Oct 30, 2020
8a9a574
Optimise mcontext init by avoiding excessive zeroing. (#4499)
abhinav92003 Oct 30, 2020
feecbf1
i#1369: Use synch flush callback to enable drcachesim tracing. (#4491)
abhinav92003 Oct 30, 2020
e4cf5af
Add gdb scripts for memquery and drsymload (#4505)
derekbruening Oct 30, 2020
8195d88
Fix uninitialized var in drmemtrace offline instru (#4506)
derekbruening Oct 31, 2020
f985439
Merge branch 'master' into i2440-add-conditional-instr-macros
yury-khrustalev Nov 2, 2020
c84871d
i#2440: Remove sizes of imm operands from docstings
yury-khrustalev Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions core/ir/aarch64/instr_create.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2565,5 +2565,136 @@ enum {
*/
#define INSTR_CREATE_fmov_scalar_imm(dc, Rd, f) instr_create_1dst_1src(dc, OP_fmov, Rd, f)

/**
* Creates a CCMN (Conditional Compare Negative) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rn The source register.
* \param Op Either a 5-bit immediate (use opnd_create_immed_uint(val, OPSZ_5b)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really need to be created with the OPSZ_5b size? Could we change that and have the encoder handle any declared size? Unless there are multiple encoding variants that differ in immediate sizes, the encoder shouldn't care so long as the value fits in 5 bits? It would be best for usability to not require anyone to specify all these precise bit counts. If a bit count does have to specified, there should be a named constant like OPSZ_ccmn or sthg like x86 has for OPSZ_xlat, OPSZ_lgdt, etc., and in addition maybe this macro should take just the integer and supply the size or set the size of the opnd before passing it on or sthg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For these instructions we need an unsigned integer immediate operand. The existing API for creating such operands (opnd_create_immed_uint) requires to specify an opnd_size_t value. How do you suggest to create an operand without using opnd_create_immed_uint? Or do you suggest to change opnd_create_immed_uint?

Copy link
Contributor

Choose a reason for hiding this comment

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

Allow any size. Or additionally maybe add opnd_create_immed_uint_unsized and internally it sets OPSZ_4 or sthg and the a64 encoder ignores the size since it has no encoding decisions based on size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it is helpful to give an example of how to create a correct operand. It would be better if this macro took just an integer value, but most of INSTR_CREATE_ macros accept operands. I can remove OPSZ_5b from the doc string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can remove OPSZ_5b from the doc string.

SGTM. The other suggestions (like an _unsized version) are separate/larger scope from this PR. I'm pretty sure the a64 encoder today ignores the size completely, so the user could pass OPSZ_NA or sthg.

* to create the operand) or a source register.
* \param nzcv The 4 bit flag bit specifier
* (use opnd_create_immed_uint(val, OPSZ_4b) to create the operand).
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_ccmn(dc, Rn, Op, nzcv, cond) \
instr_create_0dst_4src(dc, OP_ccmn, Rn, Op, nzcv, cond)

/**
* Creates a CCMP (Conditional Compare) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rn The source register.
* \param Op Either a 5-bit immediate (use opnd_create_immed_uint(val, OPSZ_5b)
* to create the operand) or a source register.
* \param nzcv The 4 bit flag bit specifier
* (use opnd_create_immed_uint(val, OPSZ_4b) to create the operand).
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_ccmp(dc, Rn, Op, nzcv, cond) \
instr_create_0dst_4src(dc, OP_ccmp, Rn, Op, nzcv, cond)

/**
* Creates a CINC (Conditional Increment) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_cinc(dc, Rd, Rn, cond) \
instr_create_1dst_3src(dc, OP_csinc, Rd, Rn, Rn, opnd_invert_cond(cond))

/**
* Creates a CINV (Conditional Invert) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_cinv(dc, Rd, Rn, cond) \
instr_create_1dst_3src(dc, OP_csinv, Rd, Rn, Rn, opnd_invert_cond(cond))

/**
* Creates a CNEG (Conditional Negate) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_cneg(dc, Rd, Rn, cond) \
instr_create_1dst_3src(dc, OP_csneg, Rd, Rn, Rn, opnd_invert_cond(cond))

/**
* Creates a CSEL (Conditional Select) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The first input register.
* \param Rm The second input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_csel(dc, Rd, Rn, Rm, cond) \
instr_create_1dst_3src(dc, OP_csel, Rd, Rn, Rm, cond)

/**
* Creates a CSET (Conditional Set) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_cset(dc, Rd, cond) \
instr_create_1dst_3src(dc, OP_csinc, Rd, OPND_CREATE_ZR(Rd), OPND_CREATE_ZR(Rd), \
opnd_invert_cond(cond))

/**
* Creates a CSETM (Conditional Set Mask) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_csetm(dc, Rd, cond) \
instr_create_1dst_3src(dc, OP_csinv, Rd, OPND_CREATE_ZR(Rd), OPND_CREATE_ZR(Rd), \
opnd_invert_cond(cond))

/**
* Creates a CSINC (Conditional Select Increment) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The first input register.
* \param Rm The second input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_csinc(dc, Rd, Rn, Rm, cond) \
instr_create_1dst_3src(dc, OP_csinc, Rd, Rn, Rm, cond)

/**
* Creates a CSINV (Conditional Select Invert) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The first input register.
* \param Rm The second input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_csinv(dc, Rd, Rn, Rm, cond) \
instr_create_1dst_3src(dc, OP_csinv, Rd, Rn, Rm, cond)

/**
* Creates a CSNEG (Conditional Select Negate) instruction.
* \param dc The void * dcontext used to allocate memory for the instr_t.
* \param Rd The output register.
* \param Rn The first input register.
* \param Rm The second input register.
* \param cond A 4-bit immediate value for condition
* (use opnd_create_cond(val) to create the operand).
*/
#define INSTR_CREATE_csneg(dc, Rd, Rn, Rm, cond) \
instr_create_1dst_3src(dc, OP_csneg, Rd, Rn, Rm, cond)

/* DR_API EXPORT END */
#endif /* INSTR_CREATE_H */
28 changes: 28 additions & 0 deletions core/ir/opnd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1777,6 +1777,34 @@ opnd_t
opnd_create_base_disp_aarch64(reg_id_t base_reg, reg_id_t index_reg,
dr_extend_type_t extend_type, bool scaled, int disp,
dr_opnd_flags_t flags, opnd_size_t size);
DR_API
/**
* Creates an unsigned immediate integer operand for condition defined by enum
* constant \p cond of type #dr_pred_type_t.
* \note AArch64-only.
*/
opnd_t
opnd_create_cond(int cond);

DR_API
/**
* Assumes the operand is an unsigned immediate 4-bit integer and returns a
* #dr_pred_type_t constant corresponding to the value of the operand.
* \note AArch64-only.
*/
int
opnd_get_cond(opnd_t opnd);

DR_API
/**
* Assumes the operand is an unsigned immediate 4-bit integer and returns an
* operand which contains inverted condition (see #dr_pred_type_t).
* \note Does not support AL and NV conditions.
* \note AArch64-only.
*/
opnd_t
opnd_invert_cond(opnd_t opnd);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs some thought as to how to integrate with existing interfaces and code, especially with ARM code.

The API already has instr_invert_predicate. I would agree it seems less of an instr-level operation, but it would be confusing to have two different sets of interfaces.

The API also already has predicates passed as dr_pred_type_t to things like XINST_CREATE_jump_cond in arm/instr_create.h. If a new type of operand, even if it's an immediate underneath is created, it would be best to use it everywhere. Whatever is decided, it seems best to have the same approach for all platforms, rather than special AArch64-only functions that don't match ARM functions yet do the same thing. That makes for confusing interfaces and complex cross-platform clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be a very big change to move all the condition-related interfaces that currently exist on the level of instructions to the level of operands. I don't think I know enough about other platforms to come up with a uniform solution, and I don't know if such change actually makes sense for x86 (after all instr_invert_predicate was create for some reason).

Anyway, I'd like to do it in steps rather than in a single big pull request which is very difficult to merge. At least some guidance, suggestions or ideas would be helpful.

I would start with #4502, but again as a separate patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now I could remove opnd_invert_cond and use instr_invert_predicate around instr_create_... to flip the operand, however I'm not sure if instr_invert_predicate is actually implemented for AArch64.

Copy link
Contributor

Choose a reason for hiding this comment

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

See https://dynamorio.org/dynamorio_docs/API_BT.html#sec_predication which lists one key current use of predicates/conditions:

  • An instruction-level modifier that is not an operand: instr_{get,set}_predicate; INSTR_PRED short form. While perhaps this could have been an operand (along with other things like condition code effects: tradeoffs with matching ISA conventions), it is not. Yet we want to be able to invert these predicates too. The current instr_invert_predicate does not actually operate on instr_t: it operates on dr_pred_type_t and thus operates on the instr-level predicates as well as operand-level.

Another current use, similar to the macros added here:

  • XINST_CREATE_jump_cond(). This, on x86, arm, and aarch64, takes in a dr_pred_type_t-typed value. I would think that the signature/operation of this instruction should match things like the INSTR_CREATE_ccmn added here. One shouldn't take a dr_pred_type_t while the other takes an opnd_t. Maybe the argument could be made that we should change the XINST_CREATE_jump_cond to take an opnd_t of this new type: though note that the condition turns into different opcodes on x86, the instruction-level predicate field on arm, and an operand on aarch64, so they are not all treating it as an operand. See also other high-level discussions on what should constitute an operand versus an opcode or sub-opcode: i#4329: Implement effective address of DC cache operations #4386 (comment). Perhaps these aarch64 comparison should be using separate opcodes, rather than operands that change behavior which goes against what operands are supposed to do. Having a single opcode doing all these different comparisons is sort of like having one opcode "arithmetic" with an operand that says whether it's an add or a subtract or whatnot, instead of separate add and subtract opcodes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Continuing in the thread of the last few sentences: xref other issues where we want to split up the current aarch64 opcodes which have behavior-controlling integer operands which are perhaps better as separate opcodes rather than operands: #4388, #4393.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WDYT about splitting up CCMN and CCMP?

Why only these instructions? All conditional instructions in AArch64 (all mentioned in this PR) have cond operand.

Also, in my opinion, inventing non-documented opcodes may create extra confusion. If some notion is not common to the rest of the architectures it's not the reason to disallow low-level access as documented. I think in DynamoRIO, when we work at the level of instructions and operands, we don't really have the benefit of "write once run everywhere". In my understanding, the point of the tool is to allow working on the lowest possible level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior comes from the opcode, not the operands.

How about compare and swap instructions (e.g. CAS)? The behaviour depends on the value of the register being equal to the value loaded from memory. It is similar to the conditional instructions, although in the latter case the comparison has happened before.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be useful to think of the pseudocode that specifies instruction behaviors. CCMP is specified by one pseudocode function, and the operands are inputs to that function. That single behavior function acts uniformly on all values of the operands. So it makes sense to treat it as one instruction, rather than single out one operand and split it into one instruction for each value of that operand.

The situation with SYS instructions is very different (similar to CDP instructions in 32-bit Arm) - they really are separate functions, selected by a function code, and it does make much more sense to split them out, at least when their specific functions are being differently handled by DynamoRIO.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AssadHashmi WDYT about splitting up CCMN and CCMP?

If I understand correctly we have 3 choices:

1 - Create a new operand for cond.
2 - Split out the opcodes into specific cond types, e.g. ccmp_eq, ccmp_ne etc.
3 - Use existing dr_pred_type_t as in XINST_CREATE_jump_cond (alias for INSTR_CREATE_bcond).

1 requires a non-trivial amount of work for a type which has essentially the same intent as dr_pred_type_t.

2 is not consistent with AArch64's existing codec. We may run into problems as I did in #4386

3 would be my choice. It makes use of an existing type with the same intent which has already been implemented and does not involve a lot of codec reworking.
e.g.

#define INSTR_CREATE_ccmp(dc, Rn, Op, nzcv, cond) \
    (INSTR_PRED(instr_create_0dst_3src(dc, OP_ccmp, Rn, Op, nzcv), (cond)))

Which users can call with:

INSTR_CREATE_ccmp(dc, opnd_create_reg(DR_REG_X1),
        opnd_create_immed_int(0b01010, OPSZ_5b),
        opnd_create_immed_int(0b1111, OPSZ_4b), DR_PRED_EQ);

Copy link
Contributor

Choose a reason for hiding this comment

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

@AssadHashmi highlights what is perhaps the main issue here: using a simple and consistent set of abstractions and types in the IR. If we take each encoded behavior immediate integer that AArch64 presents as an operand (but I would call a sub-opcode and another ISA might well make part of the opcode, such as x86 conditional moves) and place it as a separate operand in DR's IR, we then need to make a new operand type with a new set of rules and enums and handling code and introduce a new concept to tool writers for what could logically fit into existing concepts. We could end up with a dozen new operand types without much benefit but incurring non-trivial costs. Using an existing IR concept keeps the IR simpler and cleaner and more consistent.

we don't really have the benefit of "write once run everywhere".

We can make it as easy as possible, though. It does make a difference: each IR choice that lets a tool or analysis library be more platform-agnostic makes tool writing easier.

In my understanding, the point of the tool is to allow working on the lowest possible level.

None of any of the discussion here has any bearing on whether a tool writer can generate any precise machine code desired: we're only talking about how to represent ISA concepts in the DR IR. None of the choices affect whether the ISA concept can be represented or whether the tool writer can generate it. And while the IR should be more abstracted and cross-platform, we can and do have layers that are closer to the assembly conventions of that ISA: the INSTR_CREATE_ macros try to look more like assembly, disassembly has both an abstracted DR format and an ISA format, and we'd love to have an even closer layer where assembler strings are converted into IR (can't seem to find it in the tracker...#4510) when writing code for a specific machine. But code analyzing and manipulating at the IR level should have as few types and special operand concepts as possible.


#endif

DR_API
Expand Down
80 changes: 79 additions & 1 deletion core/ir/opnd_shared.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,85 @@ opnd_create_base_disp_aarch64(reg_id_t base_reg, reg_id_t index_reg,
CLIENT_ASSERT(false, "opnd_create_base_disp_aarch64: invalid extend type");
return opnd;
}
#endif

opnd_t
opnd_create_cond(int cond)
{
/* FIXME i#1569: Move definition of dr_pred_type_t to opnd.h for AArch64
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create an issue separate from the AArch64 master issue #1569 to track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created: #4502.

* to avoid using int instead of dr_pred_type_t */
switch (cond) {
case DR_PRED_EQ: return opnd_create_immed_uint(0b0000, OPSZ_4b);
case DR_PRED_NE: return opnd_create_immed_uint(0b0001, OPSZ_4b);
case DR_PRED_CS: return opnd_create_immed_uint(0b0010, OPSZ_4b);
case DR_PRED_CC: return opnd_create_immed_uint(0b0011, OPSZ_4b);
case DR_PRED_MI: return opnd_create_immed_uint(0b0100, OPSZ_4b);
case DR_PRED_PL: return opnd_create_immed_uint(0b0101, OPSZ_4b);
case DR_PRED_VS: return opnd_create_immed_uint(0b0110, OPSZ_4b);
case DR_PRED_VC: return opnd_create_immed_uint(0b0111, OPSZ_4b);
case DR_PRED_HI: return opnd_create_immed_uint(0b1000, OPSZ_4b);
case DR_PRED_LS: return opnd_create_immed_uint(0b1001, OPSZ_4b);
case DR_PRED_GE: return opnd_create_immed_uint(0b1010, OPSZ_4b);
case DR_PRED_LT: return opnd_create_immed_uint(0b1011, OPSZ_4b);
case DR_PRED_GT: return opnd_create_immed_uint(0b1100, OPSZ_4b);
case DR_PRED_LE: return opnd_create_immed_uint(0b1101, OPSZ_4b);
case DR_PRED_AL: return opnd_create_immed_uint(0b1110, OPSZ_4b);
case DR_PRED_NV: return opnd_create_immed_uint(0b1111, OPSZ_4b);
default:
CLIENT_ASSERT(false, "invalid condition constant");
return opnd_create_null();
}
}

int
opnd_get_cond(opnd_t opnd)
{
/* FIXME i#1569: Move definition of dr_pred_type_t to opnd.h for AArch64
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

* to avoid using int instead of dr_pred_type_t */
switch (((uint64)opnd_get_immed_int(opnd) & 0xfu)) {
case 0b0000: return DR_PRED_EQ;
case 0b0001: return DR_PRED_NE;
case 0b0010: return DR_PRED_CS;
case 0b0011: return DR_PRED_CC;
case 0b0100: return DR_PRED_MI;
case 0b0101: return DR_PRED_PL;
case 0b0110: return DR_PRED_VS;
case 0b0111: return DR_PRED_VC;
case 0b1000: return DR_PRED_HI;
case 0b1001: return DR_PRED_LS;
case 0b1010: return DR_PRED_GE;
case 0b1011: return DR_PRED_LT;
case 0b1100: return DR_PRED_GT;
case 0b1101: return DR_PRED_LE;
case 0b1110: return DR_PRED_AL;
case 0b1111: return DR_PRED_NV;
}
return DR_PRED_NONE;
}

opnd_t
opnd_invert_cond(opnd_t opnd)
{
switch (opnd_get_cond(opnd)) {
case DR_PRED_EQ: return opnd_create_cond(DR_PRED_NE);
case DR_PRED_NE: return opnd_create_cond(DR_PRED_EQ);
case DR_PRED_CS: return opnd_create_cond(DR_PRED_CC);
case DR_PRED_CC: return opnd_create_cond(DR_PRED_CS);
case DR_PRED_MI: return opnd_create_cond(DR_PRED_PL);
case DR_PRED_PL: return opnd_create_cond(DR_PRED_MI);
case DR_PRED_VS: return opnd_create_cond(DR_PRED_VC);
case DR_PRED_VC: return opnd_create_cond(DR_PRED_VS);
case DR_PRED_HI: return opnd_create_cond(DR_PRED_LS);
case DR_PRED_LS: return opnd_create_cond(DR_PRED_HI);
case DR_PRED_GE: return opnd_create_cond(DR_PRED_LT);
case DR_PRED_LT: return opnd_create_cond(DR_PRED_GE);
case DR_PRED_GT: return opnd_create_cond(DR_PRED_LE);
case DR_PRED_LE: return opnd_create_cond(DR_PRED_GT);
default:
CLIENT_ASSERT(false, "invalid condition constant");
return opnd_create_null();
}
}
#endif /* AARCH64 */

#undef opnd_get_base
#undef opnd_get_disp
Expand Down
Loading