i#751: IBL optimization

Reviewer: bruening@google.com
i#751: IBL optimization

i#751: IBL optimization.

Spill xax, xcx, and xbx to r8, r9, and r10, respectively.

Note: this breaks win32.x86_to_x64 test because r8 is clobbered (i#865).

[Reid Kleckner (google), 2012-08-17 22:45:12]

I thought you were going to fix the test to make it not fail when doing
translation?


On Fri, Aug 17, 2012 at 6:41 PM, <yangt@google.com> wrote:

> Reviewers: bruening,
>
> Description:
> Reviewer: bruening@google.com
> i#751: IBL optimization
>
> i#751: IBL optimization.
>
> Spill xax, xcx, and xbx to r8, r9, and r10, respectively.
>
> Note: this breaks win32.x86_to_x64 test because r8 is clobbered (i#865).
>
>
> Please review this at
http://codereview.appspot.com/**6453142/<http://codereview.appspot.com/6453142/>
>
> Affected files:
>   M trunk/core/x86/arch.h
>   M trunk/core/x86/arch_exports.h
>   M trunk/core/x86/emit_utils.c
>   M trunk/core/x86/instr.c
>   M trunk/core/x86/instr.h
>   M trunk/core/x86/interp.c
>   M trunk/core/x86/mangle.c
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "DynamoRIO Devs" group.
> To post to this group, send email to
dynamorio-devs@googlegroups.**com<dynamorio-devs@googlegroups.com>
> .
> To unsubscribe from this group, send email to dynamorio-devs+unsubscribe@*
> *googlegroups.com <dynamorio-devs%2Bunsubscribe@googlegroups.com>.
> For more options, visit this group at http://groups.google.com/**
>
group/dynamorio-devs?hl=en<http://groups.google.com/group/dynamorio-devs?hl=en>
> .
>
>

[Reid Kleckner (google), 2012-08-20 19:57:33]

I want to get Derek's opinion in the meeting today on whether we should try to
clean up the spilling code now, or file an issue for later.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2741: *pc = 0x49; pc++;
This is REX_W (or something), right?  I'd add a RAW_PREFIX_rex_w to the
RAW_PREFIX_* enum in instr.h.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2746: pc = insert_restore_xax(dcontext, pc,
f->flags, IBL_EFLAGS_IN_TLS(),
OK, I considered folding these into insert_restore_x[ac]x, but then they
wouldn't match insert_save_xax exactly.  Hm.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:4319: PRE(ilist, where, RESTORE_FROM_REG(dcontext,
REG_XAX, REG_R8));
:(  I see lots of variations on:

if (IF_X64_ELSE(x86_to_x64, false)) {
  /* save to r8-10 */
} else if (tls) {
  /* save to tls */
} else if (absolute) {
  /* save to dcontext offset */
}

I would *really* like to clean this up to:
SAVE_IBL_REG(dcontext, tls, x86_to_x64, REG_XAX);

But I'm not going to ask you to do it.  I'll go file a bug about it and assign
it to myself.

[bruening, 2012-08-24 03:59:19]

mostly style issues but I'll take another look in a final round

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/arch_exports.h
File trunk/core/x86/arch_exports.h (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/arch_exports.h...
trunk/core/x86/arch_exports.h:969: (FRAG_IS_X86_TO_X64(flags) ?
SIZE64_MOV_R8_TO_XAX : \
can you make a new define SIZE64_MOV_R9_TO_XCX (or XBX: often XAX has a special,
shorter encoding, hence the division).  size may be the same but it looks odd to
see that XAX define here

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2741: *pc = 0x49; pc++;
use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2754: *pc = 0x49; pc++;
use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2771: *pc = 0x49; pc++;
use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2773: *pc = 0xc9; pc++;
ok this is here 3 times (the 3-step *pc sequence).  can you make a function for
these 3.  for the 0xc0 and 0xc9 maybe better to use MODRM_BYTE() and construct
it so it's clearer.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:3810: APP(ilist, RESTORE_FROM_REG(dcontext, REG_XCX,
REG_R9));
hmmm.  there are a few ways to perhaps make this a little cleaner: maybe a
restore_indirect_branch_spill() or sthg, and IBL_REG to indirect xcx.  but let's
do that later.  can you add an XXX perhaps.  let's just get your commit in. 
it's not like emit_utils.c is the paragon of clean code ;)

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:5763: * mcontext for private IBL routines (unless
private_ib_in_tls is set)
can you add to the comment

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6106: RESTORE_FROM_TLS(dcontext, REG_XBX,
MANGLE_FAR_SPILL_SLOT));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6128: RESTORE_FROM_TLS(dcontext, REG_XBX,
MANGLE_FAR_SPILL_SLOT));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6569: OPND_CREATE_INT32(1));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6769: OPND_CREATE_INT32(0)));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6864: (dcontext,
OPND_TLS_FIELD(MANGLE_XCX_SPILL_SLOT), opnd_create_reg(REG_XCX)));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/instr.h
File trunk/core/x86/instr.h (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/instr.h#newcod...
trunk/core/x86/instr.h:3533: instr_t * instr_create_restore_from_reg(dcontext_t
*dcontext, reg_id_t reg1, reg_id_t reg2);
style: line too long

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c
File trunk/core/x86/interp.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:3932: opnd_create_reg(REG_XAX)));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:3958: (MANGLE_XCX_SPILL_SLOT))));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:4812: (os_tls_offset(MANGLE_XCX_SPILL_SLOT))));
code style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:4891: (dcontext, opnd_create_reg(REG_XAX),
why not put next_tag into r9 and cmp to r9, avoiding the spill to TLS and cmp
through TLS

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c
File trunk/core/x86/mangle.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:1997: XBX_OFFSET));
style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2011: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2251: XBX_OFFSET));
style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2332: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
style violation: both bodies need {}

ok there are a lot of these: how about a helper function

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2464: XBX_OFFSET));
style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2504: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
style violation: both bodies need {}

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2703: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
style violation: both bodies need {}

[yangt, 2012-08-24 20:25:16]

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/arch_exports.h
File trunk/core/x86/arch_exports.h (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/arch_exports.h...
trunk/core/x86/arch_exports.h:969: (FRAG_IS_X86_TO_X64(flags) ?
SIZE64_MOV_R8_TO_XAX : \
On 2012/08/24 03:59:19, bruening wrote:
> can you make a new define SIZE64_MOV_R9_TO_XCX (or XBX: often XAX has a
special,
> shorter encoding, hence the division).  size may be the same but it looks odd
to
> see that XAX define here

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2741: *pc = 0x49; pc++;
On 2012/08/24 03:59:19, bruening wrote:
> use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2746: pc = insert_restore_xax(dcontext, pc,
f->flags, IBL_EFLAGS_IN_TLS(),
On 2012/08/20 19:57:33, Reid Kleckner (google) wrote:
> OK, I considered folding these into insert_restore_x[ac]x, but then they
> wouldn't match insert_save_xax exactly.  Hm.

Added insert_restore_register().

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2754: *pc = 0x49; pc++;
On 2012/08/24 03:59:19, bruening wrote:
> use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2771: *pc = 0x49; pc++;
On 2012/08/24 03:59:19, bruening wrote:
> use REX_PREFIX_BASE_OPCODE | REX_PREFIX_W_OPFLAG | REX_PREFIX_B_OPFLAG

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:2773: *pc = 0xc9; pc++;
On 2012/08/24 03:59:19, bruening wrote:
> ok this is here 3 times (the 3-step *pc sequence).  can you make a function
for
> these 3.  for the 0xc0 and 0xc9 maybe better to use MODRM_BYTE() and construct
> it so it's clearer.

Added insert_restore_register().

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:3810: APP(ilist, RESTORE_FROM_REG(dcontext, REG_XCX,
REG_R9));
On 2012/08/24 03:59:19, bruening wrote:
> hmmm.  there are a few ways to perhaps make this a little cleaner: maybe a
> restore_indirect_branch_spill() or sthg, and IBL_REG to indirect xcx.  but
let's
> do that later.  can you add an XXX perhaps.  let's just get your commit in. 
> it's not like emit_utils.c is the paragon of clean code ;)

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:5763: * mcontext for private IBL routines (unless
private_ib_in_tls is set)
On 2012/08/24 03:59:19, bruening wrote:
> can you add to the comment

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6106: RESTORE_FROM_TLS(dcontext, REG_XBX,
MANGLE_FAR_SPILL_SLOT));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6128: RESTORE_FROM_TLS(dcontext, REG_XBX,
MANGLE_FAR_SPILL_SLOT));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6569: OPND_CREATE_INT32(1));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6769: OPND_CREATE_INT32(0)));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/emit_utils.c#n...
trunk/core/x86/emit_utils.c:6864: (dcontext,
OPND_TLS_FIELD(MANGLE_XCX_SPILL_SLOT), opnd_create_reg(REG_XCX)));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/instr.h
File trunk/core/x86/instr.h (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/instr.h#newcod...
trunk/core/x86/instr.h:3533: instr_t * instr_create_restore_from_reg(dcontext_t
*dcontext, reg_id_t reg1, reg_id_t reg2);
On 2012/08/24 03:59:19, bruening wrote:
> style: line too long

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c
File trunk/core/x86/interp.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:3932: opnd_create_reg(REG_XAX)));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:3958: (MANGLE_XCX_SPILL_SLOT))));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:4812: (os_tls_offset(MANGLE_XCX_SPILL_SLOT))));
On 2012/08/24 03:59:19, bruening wrote:
> code style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/interp.c#newco...
trunk/core/x86/interp.c:4891: (dcontext, opnd_create_reg(REG_XAX),
On 2012/08/24 03:59:19, bruening wrote:
> why not put next_tag into r9 and cmp to r9, avoiding the spill to TLS and cmp
> through TLS

Done. Using r10.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c
File trunk/core/x86/mangle.c (right):

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:1997: XBX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2011: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2251: XBX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2332: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}
> 
> ok there are a lot of these: how about a helper function

Added SAVE_TO_DC_OR_TLS_OR_REG macro.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2464: XBX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2504: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

http://codereview.appspot.com/6453142/diff/4001/trunk/core/x86/mangle.c#newco...
trunk/core/x86/mangle.c:2703: MANGLE_XCX_SPILL_SLOT, XCX_OFFSET));
On 2012/08/24 03:59:19, bruening wrote:
> style violation: both bodies need {}

Done.

[bruening, 2012-08-24 22:09:40]

LGTM

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/emit_utils.c#...
trunk/core/x86/emit_utils.c:2715: * so we can use reg_get_bits(reg) for both reg
and rm.
hmm, to me seems cleaner to pass in r8/r9

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/interp.c
File trunk/core/x86/interp.c (right):

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/interp.c#newc...
trunk/core/x86/interp.c:4891: } else {
wait: what about x86_mode?  not supposed to get here I guess.  assert?

[yangt, 2012-08-27 14:56:09]

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/emit_utils.c
File trunk/core/x86/emit_utils.c (right):

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/emit_utils.c#...
trunk/core/x86/emit_utils.c:2715: * so we can use reg_get_bits(reg) for both reg
and rm.
On 2012/08/24 22:09:40, bruening wrote:
> hmm, to me seems cleaner to pass in r8/r9

Done.

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/interp.c
File trunk/core/x86/interp.c (right):

http://codereview.appspot.com/6453142/diff/11002/trunk/core/x86/interp.c#newc...
trunk/core/x86/interp.c:4891: } else {
On 2012/08/24 22:09:40, bruening wrote:
> wait: what about x86_mode?  not supposed to get here I guess.  assert?

Done.