From c87f9d80916c0728dc847db3956a707c8292dfdc Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Thu, 14 Dec 2017 00:58:01 -0500 Subject: [PATCH] i#2001 trace perf: add drutil_insert_get_mem_addr_ex() Adds drutil_insert_get_mem_addr_ex() which returns whether the passed-in extra scratch register was used. Adds a test. Leverages the new function to avoid redundant loads in drcachesim. Issue: #2001 --- api/docs/release.dox | 3 +- clients/drcachesim/tracer/instru.cpp | 13 +++-- clients/drcachesim/tracer/instru.h | 5 +- clients/drcachesim/tracer/instru_offline.cpp | 12 ++-- clients/drcachesim/tracer/instru_online.cpp | 11 ++-- ext/drutil/drutil.c | 57 +++++++++++++++---- ext/drutil/drutil.h | 12 +++- .../tests/client-interface/drutil-test.dll.c | 22 +++++-- 8 files changed, 100 insertions(+), 35 deletions(-) diff --git a/api/docs/release.dox b/api/docs/release.dox index e68f6fe02c3..fc0fa703cf2 100644 --- a/api/docs/release.dox +++ b/api/docs/release.dox @@ -1,5 +1,5 @@ /* ****************************************************************************** - * Copyright (c) 2010-2017 Google, Inc. All rights reserved. + * Copyright (c) 2010-2018 Google, Inc. All rights reserved. * Copyright (c) 2011 Massachusetts Institute of Technology All rights reserved. * Copyright (c) 2008-2010 VMware, Inc. All rights reserved. * ******************************************************************************/ @@ -220,6 +220,7 @@ Further non-compatibility-affecting changes include: facilitate using drfrontendlib by itself. - Added instr_is_string_op() and instr_is_rep_string_op(). - Added dr_app_recurlock_lock(). + - Added drutil_insert_get_mem_addr_ex(). **************************************************
diff --git a/clients/drcachesim/tracer/instru.cpp b/clients/drcachesim/tracer/instru.cpp index 616096c1059..72b88a4504e 100644 --- a/clients/drcachesim/tracer/instru.cpp +++ b/clients/drcachesim/tracer/instru.cpp @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2016-2017 Google, Inc. All rights reserved. + * Copyright (c) 2016-2018 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -110,13 +110,18 @@ instru_t::instr_is_flush(instr_t *instr) void instru_t::insert_obtain_addr(void *drcontext, instrlist_t *ilist, instr_t *where, - reg_id_t reg_addr, reg_id_t reg_scratch, opnd_t ref) + reg_id_t reg_addr, reg_id_t reg_scratch, opnd_t ref, + OUT bool *scratch_used) { bool ok; - if (opnd_uses_reg(ref, reg_scratch)) + if (opnd_uses_reg(ref, reg_scratch)) { drreg_get_app_value(drcontext, ilist, where, reg_scratch, reg_scratch); + if (scratch_used != NULL) + *scratch_used = true; + } if (opnd_uses_reg(ref, reg_addr)) drreg_get_app_value(drcontext, ilist, where, reg_addr, reg_addr); - ok = drutil_insert_get_mem_addr(drcontext, ilist, where, ref, reg_addr, reg_scratch); + ok = drutil_insert_get_mem_addr_ex(drcontext, ilist, where, ref, reg_addr, + reg_scratch, scratch_used); DR_ASSERT(ok); } diff --git a/clients/drcachesim/tracer/instru.h b/clients/drcachesim/tracer/instru.h index f15c1deafe2..ff81bf288ac 100644 --- a/clients/drcachesim/tracer/instru.h +++ b/clients/drcachesim/tracer/instru.h @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2016-2017 Google, Inc. All rights reserved. + * Copyright (c) 2016-2018 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -97,7 +97,8 @@ class instru_t bool repstr_expanded = false); static bool instr_is_flush(instr_t *instr); virtual void insert_obtain_addr(void *drcontext, instrlist_t *ilist, instr_t *where, - reg_id_t reg_addr, reg_id_t reg_scratch, opnd_t ref); + reg_id_t reg_addr, reg_id_t reg_scratch, opnd_t ref, + OUT bool *scratch_used = NULL); protected: void (*insert_load_buf_ptr)(void *, instrlist_t *, instr_t *, reg_id_t); diff --git a/clients/drcachesim/tracer/instru_offline.cpp b/clients/drcachesim/tracer/instru_offline.cpp index ece5355ef45..2092cf658a7 100644 --- a/clients/drcachesim/tracer/instru_offline.cpp +++ b/clients/drcachesim/tracer/instru_offline.cpp @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2016-2017 Google, Inc. All rights reserved. + * Copyright (c) 2016-2018 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -370,10 +370,12 @@ offline_instru_t::insert_save_addr(void *drcontext, instrlist_t *ilist, instr_t opnd_t ref, bool write) { int disp = adjust; - insert_obtain_addr(drcontext, ilist, where, reg_addr, reg_ptr, ref); - // drutil_insert_get_mem_addr may clobber reg_ptr, so we need to re-load reg_ptr. - // XXX i#2001: determine whether we have to and avoid it when we don't. - insert_load_buf_ptr(drcontext, ilist, where, reg_ptr); + bool reg_ptr_used; + insert_obtain_addr(drcontext, ilist, where, reg_addr, reg_ptr, ref, ®_ptr_used); + if (reg_ptr_used) { + // Re-load because reg_ptr was clobbered. + insert_load_buf_ptr(drcontext, ilist, where, reg_ptr); + } MINSERT(ilist, where, XINST_CREATE_store(drcontext, OPND_CREATE_MEMPTR(reg_ptr, disp), diff --git a/clients/drcachesim/tracer/instru_online.cpp b/clients/drcachesim/tracer/instru_online.cpp index 8c69d0060ac..7904b1b5698 100644 --- a/clients/drcachesim/tracer/instru_online.cpp +++ b/clients/drcachesim/tracer/instru_online.cpp @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2016-2017 Google, Inc. All rights reserved. + * Copyright (c) 2016-2018 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -189,9 +189,12 @@ online_instru_t::insert_save_addr(void *drcontext, instrlist_t *ilist, instr_t * opnd_t ref) { int disp = adjust + offsetof(trace_entry_t, addr); - insert_obtain_addr(drcontext, ilist, where, reg_addr, reg_ptr, ref); - // drutil_insert_get_mem_addr may clobber reg_ptr, so we need to reload reg_ptr - insert_load_buf_ptr(drcontext, ilist, where, reg_ptr); + bool reg_ptr_used; + insert_obtain_addr(drcontext, ilist, where, reg_addr, reg_ptr, ref, ®_ptr_used); + if (reg_ptr_used) { + // drutil_insert_get_mem_addr clobbered reg_ptr, so we need to reload reg_ptr. + insert_load_buf_ptr(drcontext, ilist, where, reg_ptr); + } MINSERT(ilist, where, XINST_CREATE_store(drcontext, OPND_CREATE_MEMPTR(reg_ptr, disp), diff --git a/ext/drutil/drutil.c b/ext/drutil/drutil.c index 9d78f969aba..e70c4b02ae9 100644 --- a/ext/drutil/drutil.c +++ b/ext/drutil/drutil.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2011-2017 Google, Inc. All rights reserved. + * Copyright (c) 2011-2018 Google, Inc. All rights reserved. * Copyright (c) 2008-2010 VMware, Inc. All rights reserved. * **********************************************************/ @@ -94,11 +94,13 @@ drutil_exit(void) #ifdef X86 static bool drutil_insert_get_mem_addr_x86(void *drcontext, instrlist_t *bb, instr_t *where, - opnd_t memref, reg_id_t dst, reg_id_t scratch); + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used); #elif defined(AARCHXX) static bool drutil_insert_get_mem_addr_arm(void *drcontext, instrlist_t *bb, instr_t *where, - opnd_t memref, reg_id_t dst, reg_id_t scratch); + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used); #endif /* X86/ARM */ @@ -112,22 +114,40 @@ drutil_insert_get_mem_addr_arm(void *drcontext, instrlist_t *bb, instr_t *where, * what gets included: memory size? perhaps should try to create a * vararg clean call arg feature to chain things together. */ +DR_EXPORT +bool +drutil_insert_get_mem_addr_ex(void *drcontext, instrlist_t *bb, instr_t *where, + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used) +{ +#if defined(X86) + return drutil_insert_get_mem_addr_x86(drcontext, bb, where, memref, dst, scratch, + scratch_used); +#elif defined(AARCHXX) + return drutil_insert_get_mem_addr_arm(drcontext, bb, where, memref, dst, scratch, + scratch_used); +#endif +} + DR_EXPORT bool drutil_insert_get_mem_addr(void *drcontext, instrlist_t *bb, instr_t *where, opnd_t memref, reg_id_t dst, reg_id_t scratch) { #if defined(X86) - return drutil_insert_get_mem_addr_x86(drcontext, bb, where, memref, dst, scratch); + return drutil_insert_get_mem_addr_x86(drcontext, bb, where, memref, dst, scratch, + NULL); #elif defined(AARCHXX) - return drutil_insert_get_mem_addr_arm(drcontext, bb, where, memref, dst, scratch); + return drutil_insert_get_mem_addr_arm(drcontext, bb, where, memref, dst, scratch, + NULL); #endif } #ifdef X86 static bool drutil_insert_get_mem_addr_x86(void *drcontext, instrlist_t *bb, instr_t *where, - opnd_t memref, reg_id_t dst, reg_id_t scratch) + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used) { if (opnd_is_far_base_disp(memref) && /* We assume that far memory references via %ds and %es are flat, @@ -156,6 +176,8 @@ drutil_insert_get_mem_addr_x86(void *drcontext, instrlist_t *bb, instr_t *where, if (scratch == DR_REG_NULL) return false; opnd_set_size(&memref, OPSZ_lea); + if (scratch_used != NULL) + *scratch_used = true; near_in_scratch = INSTR_CREATE_lea(drcontext, opnd_create_reg(scratch), memref); PRE(bb, where, near_in_scratch); @@ -196,6 +218,8 @@ drutil_insert_get_mem_addr_x86(void *drcontext, instrlist_t *bb, instr_t *where, return false; if (scratch != DR_REG_XAX && dst != DR_REG_XAX) { /* we do not have to save xax if it is saved by caller */ + if (scratch_used != NULL) + *scratch_used = true; PRE(bb, where, INSTR_CREATE_mov_ld(drcontext, opnd_create_reg(scratch), @@ -271,10 +295,12 @@ instrlist_find_app_instr(instrlist_t *ilist, instr_t *where, opnd_t opnd) static reg_id_t replace_stolen_reg(void *drcontext, instrlist_t *bb, instr_t *where, - opnd_t memref, reg_id_t dst, reg_id_t scratch) + opnd_t memref, reg_id_t dst, reg_id_t scratch, OUT bool *scratch_used) { reg_id_t reg; reg = opnd_uses_reg(memref, dst) ? scratch : dst; + if (scratch_used != NULL && reg == scratch) + *scratch_used = true; DR_ASSERT(!opnd_uses_reg(memref, reg)); dr_insert_get_stolen_reg_value(drcontext, bb, where, reg); return reg; @@ -282,7 +308,8 @@ replace_stolen_reg(void *drcontext, instrlist_t *bb, instr_t *where, static bool drutil_insert_get_mem_addr_arm(void *drcontext, instrlist_t *bb, instr_t *where, - opnd_t memref, reg_id_t dst, reg_id_t scratch) + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used) { if (!opnd_is_base_disp(memref) IF_AARCH64(&& !opnd_is_rel_addr(memref))) return false; @@ -325,10 +352,14 @@ drutil_insert_get_mem_addr_arm(void *drcontext, instrlist_t *bb, instr_t *where, } if (dst == stolen || scratch == stolen) return false; - if (base == stolen) - base = replace_stolen_reg(drcontext, bb, where, memref, dst, scratch); - else if (index == stolen) - index = replace_stolen_reg(drcontext, bb, where, memref, dst, scratch); + if (base == stolen) { + base = replace_stolen_reg(drcontext, bb, where, memref, dst, scratch, + scratch_used); + } + else if (index == stolen) { + index = replace_stolen_reg(drcontext, bb, where, memref, dst, scratch, + scratch_used); + } if (index == REG_NULL && opnd_get_disp(memref) != 0) { /* first try "add dst, base, #disp" */ instr = negated ? @@ -353,6 +384,8 @@ drutil_insert_get_mem_addr_arm(void *drcontext, instrlist_t *bb, instr_t *where, */ /* if dst is used in memref, we use scratch instead */ index = (base == dst) ? scratch : dst; + if (scratch_used != NULL && index == scratch) + *scratch_used = true; PRE(bb, where, XINST_CREATE_load_int(drcontext, opnd_create_reg(index), OPND_CREATE_INT(disp))); diff --git a/ext/drutil/drutil.h b/ext/drutil/drutil.h index fff8098e1fd..48ae546c3eb 100644 --- a/ext/drutil/drutil.h +++ b/ext/drutil/drutil.h @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2010-2017 Google, Inc. All rights reserved. + * Copyright (c) 2010-2018 Google, Inc. All rights reserved. * **********************************************************/ /* drutil: DynamoRIO Instrumentation Utilities @@ -94,6 +94,16 @@ bool drutil_insert_get_mem_addr(void *drcontext, instrlist_t *bb, instr_t *where, opnd_t memref, reg_id_t dst, reg_id_t scratch); +DR_EXPORT +/** + * Identical to drutil_insert_get_mem_addr() except it returns in the optional + * OUT parameter \p scratch_used whether or not \p scratch was written to. + */ +bool +drutil_insert_get_mem_addr_ex(void *drcontext, instrlist_t *bb, instr_t *where, + opnd_t memref, reg_id_t dst, reg_id_t scratch, + OUT bool *scratch_used); + DR_EXPORT /** * Returns the size of the memory reference \p memref in bytes. diff --git a/suite/tests/client-interface/drutil-test.dll.c b/suite/tests/client-interface/drutil-test.dll.c index 4ffcd4a5dd1..e2b7995b0bf 100644 --- a/suite/tests/client-interface/drutil-test.dll.c +++ b/suite/tests/client-interface/drutil-test.dll.c @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2012-2013 Google, Inc. All rights reserved. + * Copyright (c) 2012-2018 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -178,8 +178,8 @@ event_bb_insert(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr, reg_id_t reg2 = IF_X86_ELSE(REG_XDX, DR_REG_R1); CHECK(!instr_is_stringop_loop(instr), "rep str conversion missed one"); if (instr_writes_memory(instr)) { - for (i = 0; i < instr_num_dsts(instr); i++) { - if (opnd_is_memory_reference(instr_get_dst(instr, i))) { + for (i = 0; i < instr_num_srcs(instr); i++) { + if (opnd_is_memory_reference(instr_get_src(instr, i))) { dr_save_reg(drcontext, bb, instr, reg1, SPILL_SLOT_1); dr_save_reg(drcontext, bb, instr, reg2, SPILL_SLOT_2); /* XXX: should come up w/ some clever way to ensure @@ -187,7 +187,19 @@ event_bb_insert(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr, * it doesn't crash */ drutil_insert_get_mem_addr(drcontext, bb, instr, - instr_get_dst(instr, i), reg1, reg2); + instr_get_src(instr, i), reg1, reg2); + dr_restore_reg(drcontext, bb, instr, reg2, SPILL_SLOT_2); + dr_restore_reg(drcontext, bb, instr, reg1, SPILL_SLOT_1); + } + } + /* We test the _ex version on the dests. */ + for (i = 0; i < instr_num_dsts(instr); i++) { + if (opnd_is_memory_reference(instr_get_dst(instr, i))) { + bool used; + dr_save_reg(drcontext, bb, instr, reg1, SPILL_SLOT_1); + dr_save_reg(drcontext, bb, instr, reg2, SPILL_SLOT_2); + drutil_insert_get_mem_addr_ex(drcontext, bb, instr, + instr_get_dst(instr, i), reg1, reg2, &used); dr_restore_reg(drcontext, bb, instr, reg2, SPILL_SLOT_2); dr_restore_reg(drcontext, bb, instr, reg1, SPILL_SLOT_1); } @@ -196,5 +208,3 @@ event_bb_insert(void *drcontext, void *tag, instrlist_t *bb, instr_t *instr, check_label_data(bb); return DR_EMIT_DEFAULT; } - -