Merge pull request #755 from kmod/perf3

Allow passing NULL for empty kwargs

src/capi/typeobject.cpp

@@ -1977,7 +1977,7 @@ static PyObject* tp_new_wrapper(PyTypeObject* self, BoxedTuple* args, Box* kwds)
     // ASSERT(self->tp_new != Py_CallPythonNew, "going to get in an infinite loop");
 
     RELEASE_ASSERT(args->cls == tuple_cls, "");
-    RELEASE_ASSERT(kwds->cls == dict_cls, "");
+    RELEASE_ASSERT(!kwds || kwds->cls == dict_cls, "");
     RELEASE_ASSERT(args->size() >= 1, "");
 
     BoxedClass* subtype = static_cast<BoxedClass*>(args->elts[0]);

src/codegen/ast_interpreter.cpp

@@ -315,8 +315,12 @@ void ASTInterpreter::initArguments(int nargs, BoxedClosure* _closure, BoxedGener
     if (param_names.vararg_name)
         doStore(param_names.vararg_name, Value(getArg(i++, arg1, arg2, arg3, args), 0));
 
-    if (param_names.kwarg_name)
-        doStore(param_names.kwarg_name, Value(getArg(i++, arg1, arg2, arg3, args), 0));
+    if (param_names.kwarg_name) {
+        Box* val = getArg(i++, arg1, arg2, arg3, args);
+        if (!val)
+            val = createDict();
+        doStore(param_names.kwarg_name, Value(val, 0));
+    }
 
     assert(nargs == i);
 }
@@ -1695,7 +1699,8 @@ Box* astInterpretFunction(CLFunction* clfunc, int nargs, Box* closure, Box* gene
         std::vector<ConcreteCompilerType*> arg_types;
         for (int i = 0; i < nargs; i++) {
             Box* arg = getArg(i, arg1, arg2, arg3, args);
-            assert(arg); // only builtin functions can pass NULL args
+
+            assert(arg || i == clfunc->param_names.kwargsIndex()); // only builtin functions can pass NULL args
 
             // TODO: reenable argument-type specialization
             arg_types.push_back(UNKNOWN);

src/codegen/irgen/irgenerator.cpp

@@ -2614,7 +2614,24 @@ public:
         }
 
         if (param_names.kwarg.size()) {
-            loadArgument(internString(param_names.kwarg), arg_types[i], python_parameters[i], UnwindInfo::cantUnwind());
+            llvm::BasicBlock* starting_block = emitter.currentBasicBlock();
+            llvm::BasicBlock* isnull_bb = emitter.createBasicBlock("isnull");
+            llvm::BasicBlock* continue_bb = emitter.createBasicBlock("kwargs_join");
+
+            llvm::Value* kwargs_null
+                = emitter.getBuilder()->CreateICmpEQ(python_parameters[i], getNullPtr(g.llvm_value_type_ptr));
+            llvm::BranchInst* null_check = emitter.getBuilder()->CreateCondBr(kwargs_null, isnull_bb, continue_bb);
+
+            emitter.setCurrentBasicBlock(isnull_bb);
+            llvm::Value* created_dict = emitter.getBuilder()->CreateCall(g.funcs.createDict);
+            emitter.getBuilder()->CreateBr(continue_bb);
+
+            emitter.setCurrentBasicBlock(continue_bb);
+            llvm::PHINode* phi = emitter.getBuilder()->CreatePHI(g.llvm_value_type_ptr, 2);
+            phi->addIncoming(python_parameters[i], starting_block);
+            phi->addIncoming(created_dict, isnull_bb);
+
+            loadArgument(internString(param_names.kwarg), arg_types[i], phi, UnwindInfo::cantUnwind());
             i++;
         }
 

src/core/types.h

@@ -133,6 +133,11 @@ struct ArgPassSpec {
 
     int totalPassed() { return num_args + num_keywords + (has_starargs ? 1 : 0) + (has_kwargs ? 1 : 0); }
 
+    int kwargsIndex() const {
+        assert(has_kwargs);
+        return num_args + num_keywords + (has_starargs ? 1 : 0);
+    }
+
     uint32_t asInt() const { return *reinterpret_cast<const uint32_t*>(this); }
 
     void dump() {
@@ -162,6 +167,11 @@ struct ParamNames {
         return args.size() + (vararg.str().size() == 0 ? 0 : 1) + (kwarg.str().size() == 0 ? 0 : 1);
     }
 
+    int kwargsIndex() const {
+        assert(kwarg.str().size());
+        return args.size() + (vararg.str().size() == 0 ? 0 : 1);
+    }
+
 private:
     ParamNames() : takes_param_names(false), vararg_name(NULL), kwarg_name(NULL) {}
 };

src/gc/collector.cpp

@@ -555,7 +555,7 @@ static void callPendingFinalizers() {
         Box* box = pending_finalization_list.front();
         pending_finalization_list.pop_front();
 
-        RELEASE_ASSERT(isValidGCObject(box), "objects to be finalized should still be alive");
+        ASSERT(isValidGCObject(box), "objects to be finalized should still be alive");
 
         if (isWeaklyReferenced(box)) {
             // Callbacks for weakly-referenced objects with finalizers (if any), followed by call to finalizers.
@@ -572,7 +572,7 @@ static void callPendingFinalizers() {
         }
 
         finalize(box);
-        RELEASE_ASSERT(isValidGCObject(box), "finalizing an object should not free the object");
+        ASSERT(isValidGCObject(box), "finalizing an object should not free the object");
     }
 
     if (!initially_empty) {

src/gc/heap.cpp

@@ -128,7 +128,7 @@ void _bytesAllocatedTripped() {
 
 bool hasOrderedFinalizer(BoxedClass* cls) {
     if (cls->has_safe_tp_dealloc) {
-        assert(!cls->tp_del);
+        ASSERT(!cls->tp_del, "class \"%s\" with safe tp_dealloc also has tp_del?", cls->tp_name);
         return false;
     } else if (cls->hasNonDefaultTpDealloc()) {
         return true;

src/runtime/builtin_modules/builtins.cpp

@@ -865,7 +865,7 @@ Box* execfile(Box* _fn) {
 
 Box* print(BoxedTuple* args, BoxedDict* kwargs) {
     assert(args->cls == tuple_cls);
-    assert(kwargs->cls == dict_cls);
+    assert(!kwargs || kwargs->cls == dict_cls);
 
     Box* dest, *end;
 
@@ -873,23 +873,22 @@ Box* print(BoxedTuple* args, BoxedDict* kwargs) {
     static BoxedString* end_str = internStringImmortal("end");
     static BoxedString* space_str = internStringImmortal(" ");
 
-    auto it = kwargs->d.find(file_str);
-    if (it != kwargs->d.end()) {
+    BoxedDict::DictMap::iterator it;
+    if (kwargs && ((it = kwargs->d.find(file_str)) != kwargs->d.end())) {
         dest = it->second;
         kwargs->d.erase(it);
     } else {
         dest = getSysStdout();
     }
 
-    it = kwargs->d.find(end_str);
-    if (it != kwargs->d.end()) {
+    if (kwargs && ((it = kwargs->d.find(end_str)) != kwargs->d.end())) {
         end = it->second;
         kwargs->d.erase(it);
     } else {
         end = boxString("\n");
     }
 
-    RELEASE_ASSERT(kwargs->d.size() == 0, "print() got unexpected keyword arguments");
+    RELEASE_ASSERT(!kwargs || kwargs->d.size() == 0, "print() got unexpected keyword arguments");
 
     static BoxedString* write_str = internStringImmortal("write");
     CallattrFlags callattr_flags{.cls_only = false, .null_on_nonexistent = false, .argspec = ArgPassSpec(1) };

src/runtime/capi.cpp

@@ -1448,7 +1448,7 @@ Box* BoxedCApiFunction::__call__(BoxedCApiFunction* self, BoxedTuple* varargs, B
     STAT_TIMER(t0, "us_timer_boxedcapifunction__call__", (self->cls->is_user_defined ? 10 : 20));
     assert(self->cls == capifunc_cls);
     assert(varargs->cls == tuple_cls);
-    assert(kwargs->cls == dict_cls);
+    assert(!kwargs || kwargs->cls == dict_cls);
 
     // Kind of silly to have asked callFunc to rearrange the arguments for us, just to pass things
     // off to tppCall, but this case should be very uncommon (people explicitly asking for __call__)

src/runtime/classobj.cpp

@@ -125,7 +125,7 @@ Box* classobjCall(Box* _cls, Box* _args, Box* _kwargs) {
     assert(_args->cls == tuple_cls);
     BoxedTuple* args = static_cast<BoxedTuple*>(_args);
 
-    assert(_kwargs->cls == dict_cls);
+    assert(!_kwargs || _kwargs->cls == dict_cls);
     BoxedDict* kwargs = static_cast<BoxedDict*>(_kwargs);
 
     BoxedInstance* made = new BoxedInstance(cls);
@@ -138,7 +138,7 @@ Box* classobjCall(Box* _cls, Box* _args, Box* _kwargs) {
         if (init_rtn != None)
             raiseExcHelper(TypeError, "__init__() should return None");
     } else {
-        if (args->size() || kwargs->d.size())
+        if (args->size() || (kwargs && kwargs->d.size()))
             raiseExcHelper(TypeError, "this constructor takes no arguments");
     }
     return made;

src/runtime/descr.cpp

@@ -480,7 +480,7 @@ Box* BoxedWrapperObject::__call__(BoxedWrapperObject* self, Box* args, Box* kwds
 
     assert(self->cls == wrapperobject_cls);
     assert(args->cls == tuple_cls);
-    assert(kwds->cls == dict_cls);
+    assert(!kwds || kwds->cls == dict_cls);
 
     int flags = self->descr->wrapper->flags;
     wrapperfunc wrapper = self->descr->wrapper->wrapper;

src/runtime/dict.cpp

@@ -582,8 +582,7 @@ extern "C" int PyDict_Merge(PyObject* a, PyObject* b, int override_) noexcept {
 
 Box* dictUpdate(BoxedDict* self, BoxedTuple* args, BoxedDict* kwargs) {
     assert(args->cls == tuple_cls);
-    assert(kwargs);
-    assert(kwargs->cls == dict_cls);
+    assert(!kwargs || kwargs->cls == dict_cls);
 
     RELEASE_ASSERT(args->size() <= 1, ""); // should throw a TypeError
     if (args->size()) {
@@ -596,7 +595,7 @@ Box* dictUpdate(BoxedDict* self, BoxedTuple* args, BoxedDict* kwargs) {
         }
     }
 
-    if (kwargs->d.size())
+    if (kwargs && kwargs->d.size())
         dictMerge(self, kwargs);
 
     return None;
@@ -604,7 +603,7 @@ Box* dictUpdate(BoxedDict* self, BoxedTuple* args, BoxedDict* kwargs) {
 
 extern "C" Box* dictInit(BoxedDict* self, BoxedTuple* args, BoxedDict* kwargs) {
     int args_sz = args->size();
-    int kwargs_sz = kwargs->d.size();
+    int kwargs_sz = kwargs ? kwargs->d.size() : 0;
 
     // CPython accepts a single positional and keyword arguments, in any combination
     if (args_sz > 1)
@@ -612,11 +611,13 @@ extern "C" Box* dictInit(BoxedDict* self, BoxedTuple* args, BoxedDict* kwargs) {
 
     dictUpdate(self, args, kwargs);
 
+    if (kwargs) {
         // handle keyword arguments by merging (possibly over positional entries per CPy)
         assert(kwargs->cls == dict_cls);
 
         for (const auto& p : kwargs->d)
             self->d[p.first] = p.second;
+    }
 
     return None;
 }

src/runtime/objmodel.cpp

@@ -3361,7 +3361,7 @@ void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_name
     if (paramspec.takes_kwargs) {
         int kwargs_idx = paramspec.num_args + (paramspec.takes_varargs ? 1 : 0);
         if (rewrite_args) {
-            RewriterVar* r_kwargs = rewrite_args->rewriter->call(true, (void*)createDict);
+            RewriterVar* r_kwargs = rewrite_args->rewriter->loadConst(0);
 
             if (kwargs_idx == 0)
                 rewrite_args->arg1 = r_kwargs;
@@ -3408,7 +3408,10 @@ void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_name
         Box* kwargs
             = getArg(argspec.num_args + argspec.num_keywords + (argspec.has_starargs ? 1 : 0), arg1, arg2, arg3, args);
 
-        if (!isSubclass(kwargs->cls, dict_cls)) {
+        if (!kwargs) {
+            // TODO could try to avoid creating this
+            kwargs = new BoxedDict();
+        } else if (!isSubclass(kwargs->cls, dict_cls)) {
             BoxedDict* d = new BoxedDict();
             dictMerge(d, kwargs);
             kwargs = d;
@@ -3755,15 +3758,26 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
             // already fit, either since the type inferencer could determine that,
             // or because they only need to fit into an UNKNOWN slot.
 
-            if (npassed_args >= 1)
-                rewrite_args->arg1->addAttrGuard(offsetof(Box, cls), (intptr_t)arg1->cls);
-            if (npassed_args >= 2)
-                rewrite_args->arg2->addAttrGuard(offsetof(Box, cls), (intptr_t)arg2->cls);
-            if (npassed_args >= 3)
-                rewrite_args->arg3->addAttrGuard(offsetof(Box, cls), (intptr_t)arg3->cls);
-            for (int i = 3; i < npassed_args; i++) {
-                RewriterVar* v = rewrite_args->args->getAttr((i - 3) * sizeof(Box*), Location::any());
-                v->addAttrGuard(offsetof(Box, cls), (intptr_t)args[i - 3]->cls);
+            int kwargs_index = -1;
+            if (argspec.has_kwargs)
+                kwargs_index = argspec.kwargsIndex();
+
+            for (int i = 0; i < npassed_args; i++) {
+                Box* v = getArg(i, arg1, arg2, arg3, args);
+
+                if (i == kwargs_index) {
+                    if (v == NULL) {
+                        // I don't think this case should ever get hit currently -- the only places
+                        // we offer rewriting are places that don't have the ability to pass a NULL
+                        // kwargs.
+                        getArg(i, rewrite_args)->addGuard(0);
+                    } else {
+                        getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
+                    }
+                } else {
+                    assert(v);
+                    getArg(i, rewrite_args)->addAttrGuard(offsetof(Box, cls), (intptr_t)v->cls);
+                }
             }
             rewrite_args->args_guarded = true;
         }

src/runtime/str.cpp

@@ -1827,7 +1827,7 @@ extern "C" PyObject* _do_string_format(PyObject* self, PyObject* args, PyObject*
 
 Box* strFormat(BoxedString* self, BoxedTuple* args, BoxedDict* kwargs) {
     assert(args->cls == tuple_cls);
-    assert(kwargs->cls == dict_cls);
+    assert(!kwargs || kwargs->cls == dict_cls);
 
     Box* rtn = _do_string_format(self, args, kwargs);
     checkAndThrowCAPIException();

src/runtime/tuple.cpp

@@ -262,7 +262,7 @@ extern "C" Box* tupleNew(Box* _cls, BoxedTuple* args, BoxedDict* kwargs) {
                        getNameOfClass(cls));
 
     int args_sz = args->size();
-    int kwargs_sz = kwargs->d.size();
+    int kwargs_sz = kwargs ? kwargs->d.size() : 0;
 
     if (args_sz + kwargs_sz > 1)
         raiseExcHelper(TypeError, "tuple() takes at most 1 argument (%d given)", args_sz + kwargs_sz);

src/runtime/types.cpp

@@ -1358,7 +1358,7 @@ static Box* functionCall(BoxedFunction* self, Box* args, Box* kwargs) {
     // disallow it but it's good to know.
 
     assert(args->cls == tuple_cls);
-    assert(kwargs->cls == dict_cls);
+    assert(!kwargs || kwargs->cls == dict_cls);
     return runtimeCall(self, ArgPassSpec(0, 0, true, true), args, kwargs, NULL, NULL, NULL);
 }
 
@@ -2156,8 +2156,7 @@ public:
         AttrWrapper* self = static_cast<AttrWrapper*>(_self);
 
         assert(args->cls == tuple_cls);
-        assert(kwargs);
-        assert(kwargs->cls == dict_cls);
+        assert(!kwargs || kwargs->cls == dict_cls);
 
         RELEASE_ASSERT(args->size() <= 1, ""); // should throw a TypeError
 
@@ -2192,6 +2191,7 @@ public:
         for (auto e : *args) {
             handle(e);
         }
+        if (kwargs)
             handle(kwargs);
 
         return None;

test/tests/print_function.py

@@ -50,3 +50,5 @@ try:
 except TypeError:
     pass
 
+for i in xrange(10):
+    print()