add a secondary slowpath for callattr

tjhance · tjhance · commit f4b9906042ad · 2015-07-14T01:59:52.000-07:00
diff --git a/src/asm_writing/assembler.cpp b/src/asm_writing/assembler.cpp
@@ -155,7 +155,7 @@ int Assembler::getModeFromOffset(int offset) const {
 }
 
 void Assembler::mov(Immediate val, Register dest, bool force_64bit_load) {
-    force_64bit_load = force_64bit_load || !val.fitsInto32Bit();
+    force_64bit_load = force_64bit_load || !val.fitsInto32BitSigned();
 
     int rex = force_64bit_load ? REX_W : 0;
     int dest_idx = dest.regnum;
diff --git a/src/asm_writing/rewriter.cpp b/src/asm_writing/rewriter.cpp
@@ -345,7 +345,7 @@ void Rewriter::emitGuardJump(bool useJne) {
         }
     }
 
-    if (allArgsInPlace) {
+    if (allArgsInPlace && !should_use_second_guard_destination) {
         if (useJne) {
             assembler->jne(assembler::JumpDestination::fromStart(rewrite->getSlotSize()));
         } else {
@@ -358,6 +358,9 @@ void Rewriter::emitGuardJump(bool useJne) {
             assembler->je(assembler::JumpDestination::fromStart((1 << 31) - 2));
         }
         
+        if (should_use_second_guard_destination) {
+            assert(marked_inside_ic);
+        }
         guard_infos.emplace_back(assembler->curInstPointer(), VarLocations(args));
     }
 
@@ -366,15 +369,39 @@ void Rewriter::emitGuardJump(bool useJne) {
     }
 }
 
-void Rewriter::guardCalls() {
+void Rewriter::guardCalls(int continue_offset) {
     for (GuardInfo& guard_info : guard_infos) {
+        assembler->comment("path for guard fail");
+
         uint8_t* dest_addr = assembler->curInstPointer();
         int offset = dest_addr - guard_info.guard_jmp_addr;
         *((int*)(guard_info.guard_jmp_addr - 4)) = offset;
 
         guard_info.var_locations.arrangeAsArgs(this);
 
-        assembler->jmp(assembler::JumpDestination::fromStart(rewrite->getSlotSize()));
+        if (!should_use_second_guard_destination) {
+            assembler->jmp(assembler::JumpDestination::fromStart(rewrite->getSlotSize()));
+        } else {
+            assembler->comment("calling second slowpath");
+
+            // TODO Inefficient way to write a call
+            assembler::Register r = allocReg(assembler::R11);
+            assembler->mov(assembler::Immediate(second_slowpath), r);
+            assembler->callq(r);
+
+            assembler->comment("mark inside ic");
+
+            uintptr_t counter_addr = (uintptr_t)(&picked_slot->num_inside);
+            if (isLargeConstant(counter_addr)) {
+                assembler::Register reg = assembler::R11;
+                assembler->mov(assembler::Immediate(counter_addr), reg);
+                assembler->decl(assembler::Indirect(reg, 0));
+            } else {
+                assembler->decl(assembler::Immediate(counter_addr));
+            }
+
+            assembler->jmp(assembler::JumpDestination::fromStart(continue_offset));
+        }
     }
 }
 
@@ -1134,6 +1161,10 @@ void Rewriter::commit() {
 
     // Now, start emitting assembly; check if we're dong guarding after each.
     for (int i = 0; i < actions.size(); i++) {
+        if (second_slowpath && i == action_where_second_slowpath_starts) {
+            this->useSecondGuardDestination();
+        }
+
         actions[i].action();
 
         if (failed) {
@@ -1310,7 +1341,7 @@ bool Rewriter::finishAssembly(int continue_offset) {
 
     assembler->jmp(assembler::JumpDestination::fromStart(continue_offset));
 
-    guardCalls();
+    guardCalls(continue_offset);
 
     assembler->fillWithNops();
 
@@ -1763,6 +1794,9 @@ Rewriter::Rewriter(std::unique_ptr<ICSlotRewrite> rewrite, int num_args, const L
       const_loader(this),
       return_location(this->rewrite->returnRegister()),
       failed(false),
+      second_slowpath(NULL),
+      action_where_second_slowpath_starts(-1),
+      should_use_second_guard_destination(false),
       added_changing_action(false),
       marked_inside_ic(false) {
     initPhaseCollecting();
@@ -1775,7 +1809,6 @@ Rewriter::Rewriter(std::unique_ptr<ICSlotRewrite> rewrite, int num_args, const L
         addLocationToVar(var, l);
 
         var->is_arg = true;
-        var->arg_loc = l;
 
         args.push_back(var);
     }
@@ -2138,24 +2171,26 @@ void VarLocations::arrangeAsArgs(Rewriter* rewriter) {
 
     for (int i = 0; i < vars.size(); i++) {
         Location targetLoc = Location::forArg(i);
-        Location startLoc = Location::any();
-        assert(this->vars[i].second.size());
+        Location startLoc = Location::none();
         for (Location l : this->vars[i].second) {
             if (l == targetLoc) {
                 startLoc = l;
                 break;
             } else if (l.type == Location::Register) {
                 startLoc = l;
             } else {
-                if (startLoc == Location::any()) {
+                if (startLoc == Location::none()) {
                     startLoc = l;
                 }
             }
         }
-        assert(startLoc != Location::any());
         locs.push_back(startLoc);
-        if (startLoc.type == Location::Register) {
-            argInReg[startLoc.asRegister().regnum] = i;
+        if (startLoc != Location::none()) {
+            if (startLoc.type == Location::Register) {
+                argInReg[startLoc.asRegister().regnum] = i;
+            }
+        } else {
+            assert(this->vars[i].first->is_constant);
         }
     }
 
@@ -2170,6 +2205,9 @@ void VarLocations::arrangeAsArgs(Rewriter* rewriter) {
     };
 
     for (int i = vars.size() - 1; i >= 0; i--) {
+        if (vars[i].first->is_constant)
+            continue;
+
         Location curLoc = locs[i];
         Location targetLoc = Location::forArg(i);
         if (curLoc == targetLoc) continue;
@@ -2182,7 +2220,7 @@ void VarLocations::arrangeAsArgs(Rewriter* rewriter) {
                 assembler::Register r = getFreeReg();
                 assert(curLoc.type == Location::Scratch);
                 assembler->mov(assembler::Indirect(assembler::RSP, rewriter->rewrite->getScratchRspOffset() + curLoc.scratch_offset), r);
-                assembler->mov(r, assembler::Indirect(assembler::RSP, curLoc.stack_offset));
+                assembler->mov(r, assembler::Indirect(assembler::RSP, targetLoc.stack_offset));
                 locs[i] = targetLoc;
             }
         } else {
@@ -2193,17 +2231,32 @@ void VarLocations::arrangeAsArgs(Rewriter* rewriter) {
                 argInReg[targetLoc.asRegister().regnum] = -1;
                 locs[argInReg[r.regnum]] = r;
             }
+
             if (curLoc.type == Location::Register) {
                 assembler->mov(curLoc.asRegister(), targetLoc.asRegister());
                 argInReg[curLoc.asRegister().regnum] = -1;
             } else {
                 assert(curLoc.type == Location::Scratch);
                 assembler->mov(assembler::Indirect(assembler::RSP, rewriter->rewrite->getScratchRspOffset() + curLoc.scratch_offset), targetLoc.asRegister());
             }
+
             locs[i] = targetLoc;
             argInReg[targetLoc.asRegister().regnum] = i;
         }
     }
+
+    for (int i = vars.size() - 1; i >= 0; i--) {
+        if (vars[i].first->is_constant) {
+            Location targetLoc = Location::forArg(i);
+            if (targetLoc.type == Location::Register) {
+                assembler->mov(assembler::Immediate(vars[i].first->constant_value), targetLoc.asRegister());
+            } else {
+                // TODO inefficient
+                assembler->mov(assembler::Immediate(vars[i].first->constant_value), assembler::R11);
+                assembler->mov(assembler::R11, assembler::Indirect(assembler::RSP, targetLoc.stack_offset));
+            }
+        }
+    }
 }
 
 }
diff --git a/src/asm_writing/rewriter.h b/src/asm_writing/rewriter.h
@@ -267,7 +267,6 @@ class RewriterVar {
     bool is_constant;
 
     uint64_t constant_value;
-    Location arg_loc;
     std::pair<int /*offset*/, int /*size*/> scratch_allocation;
 
     llvm::SmallSet<std::tuple<int, uint64_t, bool>, 4> attr_guards; // used to detect duplicate guards
@@ -437,10 +436,42 @@ class Rewriter : public ICSlotRewrite::CommitHook {
     LocMap<RewriterVar*> vars_by_location;
     llvm::SmallVector<RewriterVar*, 8> args;
     llvm::SmallVector<RewriterVar*, 8> live_outs;
+    llvm::SmallVector<RewriterVar*, 8> second_slowpath_args;
 
     llvm::SmallVector<GuardInfo, 8> guard_infos;
     void emitGuardJump(bool useJne);
-    void guardCalls();
+    void guardCalls(int continue_offset);
+
+    void useSecondGuardDestination() {
+        assertPhaseEmitting();
+
+        for (RewriterVar* v : args) {
+            v->is_arg = false;
+        }
+        args = std::move(second_slowpath_args);
+        for (int i = 0; i < args.size(); i++) {
+            RewriterVar* v = args[i];
+            v->is_arg = true;
+        }
+
+        should_use_second_guard_destination = true;
+    }
+
+public:
+    void addSecondSlowpath(void* new_slowpath, const llvm::SmallVector<RewriterVar*, 8>& args) {
+        assertPhaseCollecting();
+        added_changing_action = false;
+        this->second_slowpath = new_slowpath;
+        action_where_second_slowpath_starts = actions.size();
+        second_slowpath_args = args;
+    }
+    bool hasAddedChangingAction() { return added_changing_action; }
+
+protected:
+
+    void* second_slowpath;
+    int action_where_second_slowpath_starts;
+    bool should_use_second_guard_destination;
 
     Rewriter(std::unique_ptr<ICSlotRewrite> rewrite, int num_args, LiveOutSet live_outs);
 
@@ -458,7 +489,7 @@ class Rewriter : public ICSlotRewrite::CommitHook {
                 failed = true;
                 return;
             }
-            for (RewriterVar* arg : args) {
+            for (RewriterVar* arg : (second_slowpath ? second_slowpath_args : args)) {
                 arg->uses.push_back(actions.size());
             }
             assert(!added_changing_action);
diff --git a/src/asm_writing/types.h b/src/asm_writing/types.h
@@ -163,6 +163,11 @@ struct Immediate {
         uint32_t val_32bit = (uint32_t)val;
         return val_32bit == val;
     }
+
+    bool fitsInto32BitSigned() const {
+        int64_t val = (int64_t) this->val;
+        return val >= std::numeric_limits<int32_t>::min() && val <= std::numeric_limits<int32_t>::max();
+    }
 };
 
 struct JumpDestination {
diff --git a/src/runtime/objmodel.cpp b/src/runtime/objmodel.cpp
@@ -1357,11 +1357,6 @@ Box* dataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, BoxedS
                            getTypeName(getset_descr));
         }
 
-        // Abort because right now we can't call twice in a rewrite
-        if (for_call) {
-            rewrite_args = NULL;
-        }
-
         if (rewrite_args) {
             // hmm, maybe we should write assembly which can look up the function address and call any function
             r_descr->addAttrGuard(offsetof(BoxedGetsetDescriptor, get), (intptr_t)getset_descr->get);
@@ -1669,21 +1664,6 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
 
                 // Call __get__(descr, obj, obj->cls)
                 if (_set_) {
-                    // Have to abort because we're about to call now, but there will be before more
-                    // guards between this call and the next...
-                    if (for_call) {
-#if STAT_CALLATTR_DESCR_ABORTS
-                        if (rewrite_args) {
-                            std::string attr_name = "num_callattr_descr_abort";
-                            Stats::log(Stats::getStatCounter(attr_name));
-                            logByCurrentPythonLine(attr_name);
-                        }
-#endif
-
-                        rewrite_args = NULL;
-                        REWRITE_ABORTED("");
-                    }
-
                     Box* res;
                     if (rewrite_args) {
                         CallRewriteArgs crewrite_args(rewrite_args->rewriter, r_get, rewrite_args->destination);
@@ -1809,20 +1789,6 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
         // We looked up __get__ above. If we found it, call it and return
         // the result.
         if (descr_get) {
-            // this could happen for the callattr path...
-            if (for_call) {
-#if STAT_CALLATTR_DESCR_ABORTS
-                if (rewrite_args) {
-                    std::string attr_name = "num_callattr_descr_abort";
-                    Stats::log(Stats::getStatCounter(attr_name));
-                    logByCurrentPythonLine(attr_name);
-                }
-#endif
-
-                rewrite_args = NULL;
-                REWRITE_ABORTED("");
-            }
-
             Box* res;
             if (rewrite_args) {
                 assert(_get_);
@@ -2757,6 +2723,25 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallRewrit
             rewrite_args->obj = r_val;
         }
 
+        if (rewrite_args && rewrite_args->rewriter->hasAddedChangingAction()) {
+            // It's possible that `getattrInternalEx` did some mutation operation, like a runtimeCall
+            // (Could do that if it's a descriptor). Since we would have to make guards for the runtimeCall below,
+            // we would have to abort if we don't add this new slowpath.
+            RewriterVar* r_null = rewrite_args->rewriter->loadConst(0);
+            llvm::SmallVector<RewriterVar*, 8> args;
+            args.push_back(r_val);
+            args.push_back(r_null);
+            args.push_back(rewrite_args->rewriter->loadConst(argspec.asInt()));
+            // Pass r_null because we need to pass *something* as an arg
+            // TODO let the rewriter accept NULL as an argument to mean "doesn't matter what you pass"
+            args.push_back(rewrite_args->arg1 == NULL ? r_null : rewrite_args->arg1);
+            args.push_back(rewrite_args->arg2 == NULL ? r_null : rewrite_args->arg2);
+            args.push_back(rewrite_args->arg3 == NULL ? r_null : rewrite_args->arg3);
+            args.push_back(rewrite_args->args == NULL ? r_null : rewrite_args->args);
+            args.push_back(rewrite_args->rewriter->loadConst((intptr_t)(void*)keyword_names));
+            rewrite_args->rewriter->addSecondSlowpath((void*)(runtimeCallInternal<CXX>), args);
+        }
+
         Box* rtn = runtimeCallInternal<S>(val, rewrite_args, argspec, arg1, arg2, arg3, args, keyword_names);
         assert(rtn || (S == CAPI && PyErr_Occurred()));
         return rtn;
diff --git a/test/tests/callattr_guards.py b/test/tests/callattr_guards.py
@@ -0,0 +1,26 @@
+def f(a, b):
+    print 'f is called', a, b
+
+def g(a, b, c=3):
+    print 'g is called', a, b, c
+
+counter = 0
+
+class Desc(object):
+    def __get__(self, obj, type):
+        global counter
+        counter += 1
+        if counter <= 800:
+            return f
+        else:
+            return g
+
+class C(object):
+    d = Desc()
+
+def do():
+    c = C()
+    c.d('a', 'b')
+
+for i in xrange(1000):
+    do()
diff --git a/test/tests/callattr_ics.py b/test/tests/callattr_ics.py
@@ -0,0 +1,19 @@
+# run_args: -n
+# statcheck: noninit_count('slowpath_callattr') <= 200
+
+def f(a, b):
+    print 'f is called', a, b
+
+class Desc(object):
+    def __get__(self, obj, type):
+        return f
+
+class C(object):
+    d = Desc()
+
+def do():
+    c = C()
+    c.d('a', 'b')
+
+for i in xrange(1000):
+    do()
diff --git a/test/tests/descriptors_callattr_cls_ics.py b/test/tests/descriptors_callattr_cls_ics.py
diff --git a/test/tests/descriptors_callattr_ics.py b/test/tests/descriptors_callattr_ics.py

Original file line number	Diff line number	Diff line change
`@@ -155,7 +155,7 @@ int Assembler::getModeFromOffset(int offset) const {`
`155`	`155`	`}`
`156`	`156`
`157`	`157`	`void Assembler::mov(Immediate val, Register dest, bool force_64bit_load) {`
`158`		`- force_64bit_load = force_64bit_load \|\| !val.fitsInto32Bit();`
	`158`	`+ force_64bit_load = force_64bit_load \|\| !val.fitsInto32BitSigned();`
`159`	`159`
`160`	`160`	`int rex = force_64bit_load ? REX_W : 0;`
`161`	`161`	`int dest_idx = dest.regnum;`