Merge pull request #626 from kmod/perf2

make a couple more places successfully rewrite

src/asm_writing/rewriter.cpp

@@ -726,6 +726,8 @@ void Rewriter::_call(RewriterVar* result, bool can_call_into_python, void* func_
             {
                 // this forces the register allocator to spill this register:
                 assembler::Register r2 = allocReg(l);
+                if (failed)
+                    return;
                 assert(r == r2);
                 assert(vars_by_location.count(l) == 0);
             }
@@ -886,6 +888,11 @@ void Rewriter::commit() {
     assert(!finished);
     initPhaseEmitting();
 
+    if (failed) {
+        this->abort();
+        return;
+    }
+
     static StatCounter ic_rewrites_aborted_assemblyfail("ic_rewrites_aborted_assemblyfail");
 
     auto on_assemblyfail = [&]() {
@@ -936,6 +943,11 @@ void Rewriter::commit() {
     for (int i = 0; i < actions.size(); i++) {
         actions[i].action();
 
+        if (failed) {
+            this->abort();
+            return;
+        }
+
         assertConsistent();
         if (i == last_guard_action) {
             on_done_guarding();
@@ -1116,7 +1128,9 @@ Location Rewriter::allocScratch() {
             return l;
         }
     }
-    RELEASE_ASSERT(0, "Using all %d bytes of scratch!", scratch_size);
+
+    failed = true;
+    return Location(Location::None, 0);
 }
 
 RewriterVar* Rewriter::add(RewriterVar* a, int64_t b, Location dest) {
@@ -1301,6 +1315,9 @@ void Rewriter::spillRegister(assembler::Register reg, Location preserve) {
     }
 
     Location scratch = allocScratch();
+    if (failed)
+        return;
+
     assembler::Indirect mem = indirectFor(scratch);
     assembler->mov(reg, mem);
     addLocationToVar(var, scratch);
@@ -1366,7 +1383,7 @@ assembler::Register Rewriter::allocReg(Location dest, Location otherThan) {
             spillRegister(reg);
         }
 
-        assert(vars_by_location.count(reg) == 0);
+        assert(failed || vars_by_location.count(reg) == 0);
         return reg;
     } else {
         RELEASE_ASSERT(0, "%d", dest.type);
@@ -1499,6 +1516,7 @@ Rewriter::Rewriter(ICSlotRewrite* rewrite, int num_args, const std::vector<int>&
       assembler(rewrite->getAssembler()),
       const_loader(this),
       return_location(rewrite->returnRegister()),
+      failed(false),
       added_changing_action(false),
       marked_inside_ic(false),
       last_guard_action(-1),
@@ -1626,10 +1644,10 @@ Rewriter* Rewriter::createRewriter(void* rtn_addr, int num_args, const char* deb
 
     // Horrible non-robust optimization: addresses below this address are probably in the binary (ex the interpreter),
     // so don't do the more-expensive hash table lookup to find it.
-    if (rtn_addr > (void*)0x1000000) {
+    if (rtn_addr > (void*)0x1800000) {
         ic = getICInfo(rtn_addr);
     } else {
-        assert(!getICInfo(rtn_addr));
+        ASSERT(!getICInfo(rtn_addr), "%p", rtn_addr);
     }
 
     log_ic_attempts(debug_name);

src/asm_writing/rewriter.h

@@ -332,6 +332,7 @@ private:
 
     const Location return_location;
 
+    bool failed;   // if we tried to generate an invalid rewrite.
     bool finished; // committed or aborted
 #ifndef NDEBUG
     int start_vars;
@@ -366,6 +367,10 @@ private:
         if (type == ActionType::MUTATION) {
             added_changing_action = true;
         } else if (type == ActionType::GUARD) {
+            if (added_changing_action) {
+                failed = true;
+                return;
+            }
             assert(!added_changing_action);
             last_guard_action = (int)actions.size();
         }
@@ -483,6 +488,7 @@ public:
     RewriterVar* call(bool can_call_into_python, void* func_addr, RewriterVar* arg0, RewriterVar* arg1,
                       RewriterVar* arg2);
     RewriterVar* add(RewriterVar* a, int64_t b, Location dest);
+    // Allocates n pointer-sized stack slots:
     RewriterVar* allocate(int n);
     RewriterVar* allocateAndCopy(RewriterVar* array, int n);
     RewriterVar* allocateAndCopyPlus1(RewriterVar* first_elem, RewriterVar* rest, int n_rest);

src/codegen/ast_interpreter.cpp

@@ -1295,6 +1295,8 @@ const void* interpreter_instr_addr = (void*)&ASTInterpreter::executeInner;
 
 Box* astInterpretFunction(CompiledFunction* cf, int nargs, Box* closure, Box* generator, Box* globals, Box* arg1,
                           Box* arg2, Box* arg3, Box** args) {
+    UNAVOIDABLE_STAT_TIMER(t0, "us_timer_astInterpretFunction");
+
     assert((!globals) == cf->clfunc->source->scoping->areGlobalsFromModule());
     bool can_reopt = ENABLE_REOPT && !FORCE_INTERPRETER && (globals == NULL);
     if (unlikely(can_reopt && cf->times_called > REOPT_THRESHOLD_INTERPRETER)) {

src/runtime/long.cpp

@@ -1237,6 +1237,10 @@ Box* longNonzero(BoxedLong* self) {
     return True;
 }
 
+bool longNonzeroUnboxed(BoxedLong* self) {
+    return mpz_sgn(self->n) != 0;
+}
+
 Box* longHash(BoxedLong* self) {
     if (!isSubclass(self->cls, long_cls))
         raiseExcHelper(TypeError, "descriptor '__pow__' requires a 'long' object but received a '%s'",

src/runtime/long.h

@@ -54,6 +54,8 @@ Box* longRshift(BoxedLong* lhs, Box* rhs);
 Box* longHex(BoxedLong* v);
 Box* longOct(BoxedLong* v);
 Box* longStr(BoxedLong* v);
+
+bool longNonzeroUnboxed(BoxedLong* n);
 }
 
 #endif

src/runtime/objmodel.cpp

@@ -2131,6 +2131,9 @@ extern "C" bool nonzero(Box* obj) {
         r_obj->addAttrGuard(BOX_CLS_OFFSET, (intptr_t)obj->cls);
     }
 
+    // Note: it feels silly to have all these special cases here, and we should probably be
+    // able to at least generate rewrites that are as good as the ones we write here.
+    // But for now we can't and these should be a bit faster:
     if (obj->cls == bool_cls) {
         // TODO: is it faster to compare to True? (especially since it will be a constant we can embed in the rewrite)
         if (rewriter.get()) {
@@ -2142,9 +2145,6 @@ extern "C" bool nonzero(Box* obj) {
         return bool_obj->n;
     } else if (obj->cls == int_cls) {
         if (rewriter.get()) {
-            // TODO should do:
-            // test 	%rsi, %rsi
-            // setne	%al
             RewriterVar* n = r_obj->getAttr(INT_N_OFFSET, rewriter->getReturnDestination());
             RewriterVar* b = n->toBool(rewriter->getReturnDestination());
             rewriter->commitReturning(b);
@@ -2164,18 +2164,47 @@ extern "C" bool nonzero(Box* obj) {
             rewriter->commitReturning(b);
         }
         return false;
-    }
+    } else if (obj->cls == long_cls) {
+        BoxedLong* long_obj = static_cast<BoxedLong*>(obj);
+        bool r = longNonzeroUnboxed(long_obj);
 
-    // FIXME we have internal functions calling this method;
-    // instead, we should break this out into an external and internal function.
-    // slowpath_* counters are supposed to count external calls; putting it down
-    // here gets a better representation of that.
-    // TODO move internal callers to nonzeroInternal, and log *all* calls to nonzero
-    slowpath_nonzero.log();
+        if (rewriter.get()) {
+            RewriterVar* r_rtn = rewriter->call(false, (void*)longNonzeroUnboxed, r_obj);
+            rewriter->commitReturning(r_rtn);
+        }
+        return r;
+    } else if (obj->cls == tuple_cls) {
+        BoxedTuple* tuple_obj = static_cast<BoxedTuple*>(obj);
+        bool r = (tuple_obj->ob_size != 0);
 
-    // int id = Stats::getStatId("slowpath_nonzero_" + *getTypeName(obj));
-    // Stats::log(id);
+        if (rewriter.get()) {
+            RewriterVar* r_rtn
+                = r_obj->getAttr(offsetof(BoxedTuple, ob_size))->toBool(rewriter->getReturnDestination());
+            rewriter->commitReturning(r_rtn);
+        }
+        return r;
+    } else if (obj->cls == list_cls) {
+        BoxedList* list_obj = static_cast<BoxedList*>(obj);
+        bool r = (list_obj->size != 0);
+
+        if (rewriter.get()) {
+            RewriterVar* r_rtn = r_obj->getAttr(offsetof(BoxedList, size))->toBool(rewriter->getReturnDestination());
+            rewriter->commitReturning(r_rtn);
+        }
+        return r;
+    } else if (obj->cls == str_cls) {
+        BoxedString* str_obj = static_cast<BoxedString*>(obj);
+        bool r = (str_obj->ob_size != 0);
 
+        if (rewriter.get()) {
+            RewriterVar* r_rtn
+                = r_obj->getAttr(offsetof(BoxedString, ob_size))->toBool(rewriter->getReturnDestination());
+            rewriter->commitReturning(r_rtn);
+        }
+        return r;
+    }
+
+    // TODO: rewrite these.
     static BoxedString* nonzero_str = static_cast<BoxedString*>(PyString_InternFromString("__nonzero__"));
     static BoxedString* len_str = static_cast<BoxedString*>(PyString_InternFromString("__len__"));
     // go through descriptor logic
@@ -2743,6 +2772,12 @@ extern "C" Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope,
 extern "C" Box* callattr(Box* obj, BoxedString* attr, CallattrFlags flags, ArgPassSpec argspec, Box* arg1, Box* arg2,
                          Box* arg3, Box** args, const std::vector<BoxedString*>* keyword_names) {
     STAT_TIMER(t0, "us_timer_slowpath_callattr", 10);
+#if 0
+    static uint64_t* st_id = Stats::getStatCounter("us_timer_slowpath_callattr_patchable");
+    static uint64_t* st_id_nopatch = Stats::getStatCounter("us_timer_slowpath_callattr_nopatch");
+    bool havepatch = (bool)getICInfo(__builtin_extract_return_addr(__builtin_return_address(0)));
+    ScopedStatTimer st(havepatch ? st_id : st_id_nopatch, 10);
+#endif
 
     ASSERT(gc::isValidGCObject(obj), "%p", obj);
 
@@ -2786,16 +2821,16 @@ extern "C" Box* callattr(Box* obj, BoxedString* attr, CallattrFlags flags, ArgPa
             rewrite_args.arg3 = rewriter->getArg(6);
         if (npassed_args >= 4)
             rewrite_args.args = rewriter->getArg(7);
-        // XXX whole point is to not have to do this!
         rtn = callattrInternal(obj, attr, scope, &rewrite_args, argspec, arg1, arg2, arg3, args, keyword_names);
 
         if (!rewrite_args.out_success) {
             rewriter.reset(NULL);
         } else if (rtn) {
             rewriter->commitReturning(rewrite_args.out_rtn);
+        } else if (flags.null_on_nonexistent) {
+            rewriter->commitReturning(rewriter->loadConst(0, rewriter->getReturnDestination()));
         }
     } else {
-        // XXX whole point is to not have to do this!
         rtn = callattrInternal(obj, attr, scope, NULL, argspec, arg1, arg2, arg3, args, keyword_names);
     }
 
@@ -3299,13 +3334,54 @@ static Box* callChosenCF(CompiledFunction* chosen_cf, BoxedClosure* closure, Box
         return chosen_cf->call(oarg1, oarg2, oarg3, oargs);
 }
 
+// This function exists for the rewriter: astInterpretFunction takes 9 args, but the rewriter
+// only supports calling functions with at most 6 since it can currently only pass arguments
+// in registers.
+static Box* astInterpretHelper(CompiledFunction* f, int num_args, BoxedClosure* closure, BoxedGenerator* generator,
+                               Box* globals, Box** _args) {
+    Box* arg1 = _args[0];
+    Box* arg2 = _args[1];
+    Box* arg3 = _args[2];
+    Box* args = _args[3];
+
+    return astInterpretFunction(f, num_args, closure, generator, globals, arg1, arg2, arg3, (Box**)args);
+}
+
 Box* callCLFunc(CLFunction* f, CallRewriteArgs* rewrite_args, int num_output_args, BoxedClosure* closure,
                 BoxedGenerator* generator, Box* globals, Box* oarg1, Box* oarg2, Box* oarg3, Box** oargs) {
     CompiledFunction* chosen_cf = pickVersion(f, num_output_args, oarg1, oarg2, oarg3, oargs);
 
     assert(chosen_cf->is_interpreted == (chosen_cf->code == NULL));
     if (chosen_cf->is_interpreted) {
-        UNAVOIDABLE_STAT_TIMER(t0, "us_timer_astInterpretFunction");
+        if (rewrite_args) {
+            rewrite_args->rewriter->addDependenceOn(chosen_cf->dependent_callsites);
+
+            RewriterVar::SmallVector arg_vec;
+
+            // TODO this kind of embedded reference needs to be tracked by the GC somehow?
+            // Or maybe it's ok, since we've guarded on the function object?
+            arg_vec.push_back(rewrite_args->rewriter->loadConst((intptr_t)chosen_cf, Location::forArg(0)));
+            arg_vec.push_back(rewrite_args->rewriter->loadConst((intptr_t)num_output_args, Location::forArg(1)));
+            arg_vec.push_back(rewrite_args->rewriter->loadConst((intptr_t)closure, Location::forArg(2)));
+            arg_vec.push_back(rewrite_args->rewriter->loadConst((intptr_t)generator, Location::forArg(3)));
+            arg_vec.push_back(rewrite_args->rewriter->loadConst((intptr_t)globals, Location::forArg(4)));
+
+            // Hacky workaround: the rewriter can only pass arguments in registers, so use this helper function
+            // to unpack some of the additional arguments:
+            RewriterVar* arg_array = rewrite_args->rewriter->allocate(4);
+            arg_vec.push_back(arg_array);
+            if (num_output_args >= 1)
+                arg_array->setAttr(0, rewrite_args->arg1);
+            if (num_output_args >= 2)
+                arg_array->setAttr(8, rewrite_args->arg2);
+            if (num_output_args >= 3)
+                arg_array->setAttr(16, rewrite_args->arg3);
+            if (num_output_args >= 4)
+                arg_array->setAttr(24, rewrite_args->args);
+
+            rewrite_args->out_rtn = rewrite_args->rewriter->call(true, (void*)astInterpretHelper, arg_vec);
+            rewrite_args->out_success = true;
+        }
 
         return astInterpretFunction(chosen_cf, num_output_args, closure, generator, globals, oarg1, oarg2, oarg3,
                                     oargs);