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
DynamoRIO · Jan 5, 2018 · c87f9d8 · c87f9d8
1 parent 192a026
commit c87f9d8
Show file tree

Hide file tree

Showing 8 changed files with 100 additions and 35 deletions.
diff --git 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().
 
 **************************************************
 <hr>

diff --git 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
@@ -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
@@ -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, &reg_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
@@ -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, &reg_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
@@ -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,18 +295,21 @@ 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;
 }
 
 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
@@ -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
@@ -1,5 +1,5 @@
 /* **********************************************************
- * Copyright (c) 2012-2013 Google, Inc.  All rights reserved.
+ * Copyright (c) 2012-2018 Google, Inc.  All rights reserved.
  * **********************************************************/
 
 /*
@@ -178,16 +178,28 @@ 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
                  * this gets the right address: for now just making sure
                  * 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;
 }
-
-