Merge pull request #1153 from kmod/kill_some

Add some liveness information to the cfg

src/analysis/function_analysis.cpp

@@ -136,8 +136,13 @@ public:
     bool visit_name(AST_Name* node) {
         if (node->ctx_type == AST_TYPE::Load)
             _doLoad(node->id, node);
-        else if (node->ctx_type == AST_TYPE::Store || node->ctx_type == AST_TYPE::Del
-                 || node->ctx_type == AST_TYPE::Param)
+        else if (node->ctx_type == AST_TYPE::Del) {
+            // Hack: we don't have a bytecode for temporary-kills:
+            if (node->id.s()[0] == '#')
+                return true;
+            _doLoad(node->id, node);
+            _doStore(node->id);
+        } else if (node->ctx_type == AST_TYPE::Store || node->ctx_type == AST_TYPE::Param)
             _doStore(node->id);
         else {
             ASSERT(0, "%d", node->ctx_type);
@@ -145,6 +150,7 @@ public:
         }
         return true;
     }
+
     bool visit_alias(AST_alias* node) {
         InternedString name = node->name;
         if (node->asname.s().size())

src/asm_writing/rewriter.cpp

@@ -693,6 +693,19 @@ void RewriterVar::setAttr(int offset, RewriterVar* val, SetattrType type) {
     rewriter->addAction([=]() { rewriter->_setAttr(this, offset, val); }, { this, val }, ActionType::MUTATION);
 }
 
+void RewriterVar::replaceAttr(int offset, RewriterVar* val, bool prev_nullable) {
+    RewriterVar* prev = this->getAttr(offset);
+
+    this->setAttr(offset, val, SetattrType::HANDED_OFF);
+    val->refConsumed();
+
+    if (prev_nullable) {
+        prev->setNullable(true);
+        prev->xdecref();
+    } else
+        prev->decref();
+}
+
 void Rewriter::_setAttr(RewriterVar* ptr, int offset, RewriterVar* val) {
     if (LOG_IC_ASSEMBLY)
         assembler->comment("_setAttr");

src/asm_writing/rewriter.h

@@ -155,6 +155,15 @@ public:
         REFUSED,
     };
     void setAttr(int offset, RewriterVar* other, SetattrType type = SetattrType::UNKNOWN);
+
+    // Replaces an owned ref with another one.  Does the equivalent of:
+    // Box* prev = this[offset];
+    // this[offset] = new_val;
+    // Py_[X]DECREF(prev);
+    //
+    // Calls new_val->refConsumed() for you.
+    void replaceAttr(int offset, RewriterVar* new_val, bool prev_nullable);
+
     RewriterVar* cmp(AST_TYPE::AST_TYPE cmp_type, RewriterVar* other, Location loc = Location::any());
     RewriterVar* toBool(Location loc = Location::any());
 

src/codegen/ast_interpreter.cpp

@@ -1080,6 +1080,24 @@ Value ASTInterpreter::visit_return(AST_Return* node) {
         finishJITing();
     }
 
+// Some day, we should make sure that all temporaries got deleted (and decrefed) at the right time:
+#if 0
+    bool temporaries_alive = false;
+#ifndef NDEBUG
+    for (auto&& v : getSymVRegMap()) {
+        if (v.first.s()[0] == '#' && vregs[v.second]) {
+            fprintf(stderr, "%s still alive\n", v.first.c_str());
+            temporaries_alive = true;
+        }
+    }
+
+    if (temporaries_alive)
+        source_info->cfg->print();
+
+    assert(!temporaries_alive);
+#endif
+#endif
+
     next_block = 0;
     return s;
 }
@@ -1332,15 +1350,22 @@ Value ASTInterpreter::visit_delete(AST_Delete* node) {
                         jit->emitDelName(target->id);
                     ASTInterpreterJitInterface::delNameHelper(this, target->id);
                 } else {
-                    abortJITing();
                     assert(vst == ScopeInfo::VarScopeType::FAST);
 
                     assert(getSymVRegMap().count(target->id));
                     assert(getSymVRegMap()[target->id] == target->vreg);
+
+                    if (target->id.s()[0] == '#') {
+                        assert(vregs[target->vreg] != NULL);
+                        if (jit)
+                            jit->emitKillTemporary(target->id, target->vreg);
+                    } else {
+                        abortJITing();
                         if (vregs[target->vreg] == 0) {
                             assertNameDefined(0, target->id.c_str(), NameError, true /* local_var_msg */);
                             return Value();
                         }
+                    }
 
                     Py_DECREF(vregs[target->vreg]);
                     vregs[target->vreg] = NULL;
@@ -1635,6 +1660,7 @@ Value ASTInterpreter::visit_name(AST_Name* node) {
 
     switch (node->lookup_type) {
         case ScopeInfo::VarScopeType::GLOBAL: {
+            assert(!node->is_kill);
             Value v;
             if (jit)
                 v.var = jit->emitGetGlobal(frame_info.globals, node->id.getBox());
@@ -1643,6 +1669,7 @@ Value ASTInterpreter::visit_name(AST_Name* node) {
             return v;
         }
         case ScopeInfo::VarScopeType::DEREF: {
+            assert(!node->is_kill);
             return Value(ASTInterpreterJitInterface::derefHelper(this, node->id),
                          jit ? jit->emitDeref(node->id) : NULL);
         }
@@ -1651,22 +1678,35 @@ Value ASTInterpreter::visit_name(AST_Name* node) {
             Value v;
             if (jit) {
                 bool is_live = true;
-                if (node->lookup_type == ScopeInfo::VarScopeType::FAST)
+                if (node->is_kill) {
+                    is_live = false;
+                    assert(!source_info->getLiveness()->isLiveAtEnd(node->id, current_block));
+                } else if (node->lookup_type == ScopeInfo::VarScopeType::FAST)
                     is_live = source_info->getLiveness()->isLiveAtEnd(node->id, current_block);
 
-                if (is_live)
+                if (is_live) {
+                    assert(!node->is_kill);
                     v.var = jit->emitGetLocal(node->id, node->vreg);
-                else
+                } else {
                     v.var = jit->emitGetBlockLocal(node->id, node->vreg);
+                    if (node->is_kill) {
+                        assert(node->id.s()[0] == '#');
+                        jit->emitKillTemporary(node->id, node->vreg);
+                    }
+                }
             }
 
             assert(node->vreg >= 0);
             assert(getSymVRegMap().count(node->id));
             assert(getSymVRegMap()[node->id] == node->vreg);
             Box* val = vregs[node->vreg];
+
             if (val) {
-                Py_INCREF(val);
                 v.o = val;
+                if (node->is_kill)
+                    vregs[node->vreg] = NULL;
+                else
+                    Py_INCREF(val);
                 return v;
             }
 
@@ -1674,6 +1714,7 @@ Value ASTInterpreter::visit_name(AST_Name* node) {
             RELEASE_ASSERT(0, "should be unreachable");
         }
         case ScopeInfo::VarScopeType::NAME: {
+            assert(!node->is_kill && "we might need to support this");
             Value v;
             if (jit)
                 v.var = jit->emitGetBoxedLocal(node->id.getBox());

src/codegen/baseline_jit.cpp

@@ -379,6 +379,10 @@ RewriterVar* JitFragmentWriter::emitGetBlockLocal(InternedString s, int vreg) {
     return it->second;
 }
 
+void JitFragmentWriter::emitKillTemporary(InternedString s, int vreg) {
+    emitSetLocal(s, vreg, false, imm(nullptr));
+}
+
 RewriterVar* JitFragmentWriter::emitGetBoxedLocal(BoxedString* s) {
     RewriterVar* boxed_locals = emitGetBoxedLocals();
     RewriterVar* globals = getInterp()->getAttr(ASTInterpreterJitInterface::getGlobalsOffset());
@@ -674,16 +678,13 @@ void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closur
              v);
         v->refConsumed();
     } else {
-        RewriterVar* prev = vregs_array->getAttr(8 * vreg)->setNullable(true);
-        vregs_array->setAttr(8 * vreg, v, RewriterVar::SetattrType::HANDED_OFF);
-        v->refConsumed();
-
-        // TODO With definedness analysis, we could know whether we can skip this check (definitely defined)
-        // or not even load the previous value (definitely undefined).
+        // TODO With definedness analysis, we could know whether we needed to emit an decref/xdecref/neither.
         // The issue is that definedness analysis is somewhat expensive to compute, so we don't compute it
         // for the bjit.  We could try calculating it (which would require some re-plumbing), which might help
         // but I suspect is not that big a deal as long as the llvm jit implements this kind of optimization.
-        prev->xdecref();
+        bool prev_nullable = true;
+
+        vregs_array->replaceAttr(8 * vreg, v, prev_nullable);
     }
     if (LOG_BJIT_ASSEMBLY)
         comment("BJIT: emitSetLocal() end");

src/codegen/baseline_jit.h

@@ -230,6 +230,7 @@ public:
     RewriterVar* emitExceptionMatches(RewriterVar* v, RewriterVar* cls);
     RewriterVar* emitGetAttr(RewriterVar* obj, BoxedString* s, AST_expr* node);
     RewriterVar* emitGetBlockLocal(InternedString s, int vreg);
+    void emitKillTemporary(InternedString s, int vreg);
     RewriterVar* emitGetBoxedLocal(BoxedString* s);
     RewriterVar* emitGetBoxedLocals();
     RewriterVar* emitGetClsAttr(RewriterVar* obj, BoxedString* s);

src/codegen/irgen/irgenerator.cpp

@@ -2123,6 +2123,12 @@ private:
     }
 
     void _doDelName(AST_Name* target, const UnwindInfo& unw_info) {
+        // Hack: we don't have a bytecode for temporary-kills:
+        if (target->id.s()[0] == '#') {
+            // The refcounter will automatically delete this object.
+            return;
+        }
+
         auto scope_info = irstate->getScopeInfo();
         ScopeInfo::VarScopeType vst = scope_info->getScopeTypeOfName(target->id);
         if (vst == ScopeInfo::VarScopeType::GLOBAL) {

src/core/ast.cpp

@@ -1630,7 +1630,8 @@ bool PrintVisitor::visit_suite(AST_Suite* node) {
 
 bool PrintVisitor::visit_name(AST_Name* node) {
     stream << node->id.s();
-    // printf("%s(%d)", node->id.c_str(), node->ctx_type);
+    // Uncomment this line to see which names are kills:
+    // if (node->is_kill) stream << "<k>";
     return false;
 }
 

src/core/ast.h

@@ -735,6 +735,8 @@ public:
     // the zero based index of this variable inside the vregs array. If uninitialized it's value is -1.
     int vreg;
 
+    bool is_kill = false;
+
     virtual void accept(ASTVisitor* v);
     virtual void* accept_expr(ExprVisitor* v);
 

src/core/cfg.cpp

@@ -213,12 +213,16 @@ private:
         return source->getInternedStrings().get(std::move(name));
     }
 
-    AST_Name* makeName(InternedString id, AST_TYPE::AST_TYPE ctx_type, int lineno, int col_offset = 0) {
+    AST_Name* makeName(InternedString id, AST_TYPE::AST_TYPE ctx_type, int lineno, int col_offset = 0,
+                       bool is_kill = false) {
         AST_Name* name = new AST_Name(id, ctx_type, lineno, col_offset);
+        name->is_kill = is_kill;
         return name;
     }
 
-    AST_Name* makeLoad(InternedString id, AST* node) { return makeName(id, AST_TYPE::Load, node->lineno); }
+    AST_Name* makeLoad(InternedString id, AST* node, bool is_kill = false) {
+        return makeName(id, AST_TYPE::Load, node->lineno, 0, is_kill);
+    }
 
     void pushLoopContinuation(CFGBlock* continue_dest, CFGBlock* break_dest) {
         assert(continue_dest
@@ -370,7 +374,7 @@ private:
             curblock = body_block;
             InternedString next_name(nodeName());
             pushAssign(next_name, makeCall(next_attr));
-            pushAssign(c->target, makeLoad(next_name, node));
+            pushAssign(c->target, makeLoad(next_name, node, true));
 
             for (AST_expr* if_condition : c->ifs) {
                 AST_expr* remapped = callNonzero(remapExpr(if_condition));
@@ -402,6 +406,7 @@ private:
             assert((finished_block != NULL) == (i != 0));
             if (finished_block) {
                 curblock = exit_block;
+                push_back(makeKill(iter_name));
                 pushJump(finished_block, true);
             }
             finished_block = test_block;
@@ -733,10 +738,10 @@ private:
 
         for (int i = 0; i < node->values.size() - 1; i++) {
             AST_expr* val = remapExpr(node->values[i]);
-            pushAssign(name, val);
+            pushAssign(name, _dup(val));
 
             AST_Branch* br = new AST_Branch();
-            br->test = callNonzero(_dup(val));
+            br->test = callNonzero(val);
             push_back(br);
 
             CFGBlock* was_block = curblock;
@@ -844,7 +849,7 @@ private:
                 val->col_offset = node->col_offset;
                 val->lineno = node->lineno;
                 val->left = left;
-                val->comparators.push_back(right);
+                val->comparators.push_back(_dup(right));
                 val->ops.push_back(node->ops[i]);
 
                 pushAssign(name, val);
@@ -867,7 +872,7 @@ private:
 
                 curblock = next_block;
 
-                left = _dup(right);
+                left = right;
             }
 
             pushJump(exit_block);
@@ -1303,7 +1308,7 @@ private:
         if (wrap_with_assign && (rtn->type != AST_TYPE::Name || ast_cast<AST_Name>(rtn)->id.s()[0] != '#')) {
             InternedString name = nodeName();
             pushAssign(name, rtn);
-            return makeLoad(name, node);
+            return makeLoad(name, node, true);
         } else {
             return rtn;
         }
@@ -1452,6 +1457,7 @@ public:
         ExcBlockInfo& exc_info = exc_handlers.back();
 
         curblock = exc_dest;
+        // TODO: need to clear some temporaries here
         AST_Assign* exc_asgn = new AST_Assign();
         AST_Tuple* target = new AST_Tuple();
         target->elts.push_back(makeName(exc_info.exc_type_name, AST_TYPE::Store, node->lineno));
@@ -1489,7 +1495,7 @@ public:
         auto tmp = nodeName();
         pushAssign(tmp, new AST_MakeClass(def));
         // is this name mangling correct?
-        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno));
+        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno, 0, true));
 
         return true;
     }
@@ -1511,7 +1517,7 @@ public:
         auto tmp = nodeName();
         pushAssign(tmp, new AST_MakeFunction(def));
         // is this name mangling correct?
-        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno));
+        pushAssign(source->mangleName(def->name), makeName(tmp, AST_TYPE::Load, node->lineno, node->col_offset, true));
 
         return true;
     }
@@ -1557,7 +1563,7 @@ public:
             if (a->asname.s().size() == 0) {
                 // No asname, so load the top-level module into the name
                 // (e.g., for `import os.path`, loads the os module into `os`)
-                pushAssign(internString(getTopModule(a->name.s())), makeLoad(tmpname, node));
+                pushAssign(internString(getTopModule(a->name.s())), makeLoad(tmpname, node, /* is_kill */ true));
             } else {
                 // If there is an asname, get the bottom-level module by
                 // getting the attributes and load it into asname.
@@ -1571,11 +1577,11 @@ public:
                         l = r + 1;
                         continue;
                     }
-                    pushAssign(tmpname, new AST_Attribute(makeLoad(tmpname, node), AST_TYPE::Load,
+                    pushAssign(tmpname, new AST_Attribute(makeLoad(tmpname, node, true), AST_TYPE::Load,
                                                           internString(a->name.s().substr(l, r - l))));
                     l = r + 1;
                 } while (l < a->name.s().size());
-                pushAssign(a->asname, makeLoad(tmpname, node));
+                pushAssign(a->asname, makeLoad(tmpname, node, true));
             }
         }
 
@@ -1610,13 +1616,16 @@ public:
         InternedString tmp_module_name = nodeName();
         pushAssign(tmp_module_name, import);
 
+        int i = 0;
         for (AST_alias* a : node->names) {
+            i++;
+            bool is_kill = (i == node->names.size());
             if (a->name.s() == "*") {
 
                 AST_LangPrimitive* import_star = new AST_LangPrimitive(AST_LangPrimitive::IMPORT_STAR);
                 import_star->lineno = node->lineno;
                 import_star->col_offset = node->col_offset;
-                import_star->args.push_back(makeLoad(tmp_module_name, node));
+                import_star->args.push_back(makeLoad(tmp_module_name, node, is_kill));
 
                 AST_Expr* import_star_expr = new AST_Expr();
                 import_star_expr->value = import_star;
@@ -1628,12 +1637,12 @@ public:
                 AST_LangPrimitive* import_from = new AST_LangPrimitive(AST_LangPrimitive::IMPORT_FROM);
                 import_from->lineno = node->lineno;
                 import_from->col_offset = node->col_offset;
-                import_from->args.push_back(makeLoad(tmp_module_name, node));
+                import_from->args.push_back(makeLoad(tmp_module_name, node, is_kill));
                 import_from->args.push_back(new AST_Str(a->name.s()));
 
                 InternedString tmp_import_name = nodeName();
                 pushAssign(tmp_import_name, import_from);
-                pushAssign(a->asname.s().size() ? a->asname : a->name, makeLoad(tmp_import_name, node));
+                pushAssign(a->asname.s().size() ? a->asname : a->name, makeLoad(tmp_import_name, node, true));
             }
         }
 
@@ -1681,8 +1690,13 @@ public:
     bool visit_assign(AST_Assign* node) override {
         AST_expr* remapped_value = remapExpr(node->value);
 
-        for (AST_expr* target : node->targets) {
-            pushAssign(target, _dup(remapped_value));
+        for (int i = 0; i < node->targets.size(); i++) {
+            AST_expr* val;
+            if (i == node->targets.size() - 1)
+                val = remapped_value;
+            else
+                val = _dup(remapped_value);
+            pushAssign(node->targets[i], val);
         }
         return true;
     }
@@ -1776,7 +1790,7 @@ public:
 
         InternedString node_name(nodeName());
         pushAssign(node_name, binop);
-        pushAssign(remapped_target, makeLoad(node_name, node));
+        pushAssign(remapped_target, makeLoad(node_name, node, true));
         return true;
     }
 
@@ -2050,6 +2064,13 @@ public:
         return true;
     }
 
+    AST_stmt* makeKill(InternedString name) {
+        // There might be a better way to represent this, maybe with a dedicated AST_Kill bytecode?
+        auto del = new AST_Delete();
+        del->targets.push_back(makeName(name, AST_TYPE::Del, 0, 0, false));
+        return del;
+    }
+
     bool visit_for(AST_For* node) override {
         assert(curblock);
 
@@ -2093,12 +2114,13 @@ public:
         curblock = test_false;
         pushJump(else_block);
 
+        // TODO: need to del the iter_name when break'ing out of the loop
         pushLoopContinuation(test_block, end_block);
 
         curblock = loop_block;
         InternedString next_name(nodeName());
         pushAssign(next_name, makeCall(next_attr));
-        pushAssign(node->target, makeLoad(next_name, node));
+        pushAssign(node->target, makeLoad(next_name, node, true));
 
         for (int i = 0; i < node->body.size(); i++) {
             node->body[i]->accept(this);
@@ -2130,6 +2152,7 @@ public:
         cfg->placeBlock(else_block);
         curblock = else_block;
 
+        push_back(makeKill(itername));
         for (int i = 0; i < node->orelse.size(); i++) {
             node->orelse[i]->accept(this);
             if (!curblock)
@@ -2254,16 +2277,16 @@ public:
                     caught_all = true;
                 }
 
-                AST_LangPrimitive* set_exc_info = new AST_LangPrimitive(AST_LangPrimitive::SET_EXC_INFO);
-                set_exc_info->args.push_back(makeLoad(exc_type_name, node));
-                set_exc_info->args.push_back(makeLoad(exc_value_name, node));
-                set_exc_info->args.push_back(makeLoad(exc_traceback_name, node));
-                push_back(makeExpr(set_exc_info));
-
                 if (exc_handler->name) {
                     pushAssign(exc_handler->name, _dup(exc_obj));
                 }
 
+                AST_LangPrimitive* set_exc_info = new AST_LangPrimitive(AST_LangPrimitive::SET_EXC_INFO);
+                set_exc_info->args.push_back(makeLoad(exc_type_name, node, true));
+                set_exc_info->args.push_back(makeLoad(exc_value_name, node, true));
+                set_exc_info->args.push_back(makeLoad(exc_traceback_name, node, true));
+                push_back(makeExpr(set_exc_info));
+
                 for (AST_stmt* subnode : exc_handler->body) {
                     subnode->accept(this);
                     if (!curblock)
@@ -2284,9 +2307,9 @@ public:
 
             if (!caught_all) {
                 AST_Raise* raise = new AST_Raise();
-                raise->arg0 = makeLoad(exc_type_name, node);
-                raise->arg1 = makeLoad(exc_value_name, node);
-                raise->arg2 = makeLoad(exc_traceback_name, node);
+                raise->arg0 = makeLoad(exc_type_name, node, true);
+                raise->arg1 = makeLoad(exc_value_name, node, true);
+                raise->arg2 = makeLoad(exc_traceback_name, node, true);
                 push_back(raise);
                 curblock = NULL;
             }

src/core/types.h

@@ -622,12 +622,12 @@ struct GetattrRewriteArgs;
 struct DelattrRewriteArgs;
 
 // Helper function around PyString_InternFromString:
-BoxedString* internStringImmortal(llvm::StringRef s);
+BoxedString* internStringImmortal(llvm::StringRef s) noexcept;
 
 // Callers should use this function if they can accept mortal string objects.
 // FIXME For now it just returns immortal strings, but at least we can use it
 // to start documenting the places that can take mortal strings.
-inline BoxedString* internStringMortal(llvm::StringRef s) {
+inline BoxedString* internStringMortal(llvm::StringRef s) noexcept {
     return internStringImmortal(s);
 }
 

src/runtime/objmodel.cpp

@@ -1512,13 +1512,8 @@ void Box::setattr(BoxedString* attr, BORROWED(Box*) val, SetattrRewriteArgs* rew
                     RewriterVar* r_hattrs
                         = rewrite_args->obj->getAttr(cls->attrs_offset + offsetof(HCAttrs, attr_list), Location::any());
 
-                    // Don't need to do anything: just getting it and setting it to OWNED
-                    // will tell the auto-refcount system to decref it.
-                    r_hattrs->getAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs))
-                        ->setType(RefType::OWNED);
-                    r_hattrs->setAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs), rewrite_args->attrval,
-                                      RewriterVar::SetattrType::HANDED_OFF);
-                    rewrite_args->attrval->refConsumed();
+                    r_hattrs->replaceAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs),
+                                          rewrite_args->attrval, /* prev_nullable */ false);
 
                     rewrite_args->out_success = true;
                 }

src/runtime/str.cpp

@@ -371,7 +371,7 @@ extern "C" PyObject* PyString_InternFromString(const char* s) noexcept {
     return internStringImmortal(s);
 }
 
-BoxedString* internStringImmortal(llvm::StringRef s) {
+BoxedString* internStringImmortal(llvm::StringRef s) noexcept {
     auto& entry = interned_strings[s];
     if (!entry) {
         num_interned_strings.log();

test/tests/local_lifetime.py

+from _weakref import ref
+
+# Test to make sure that we clear local variables at the right time:
+def f1():
+    def f():
+        pass
+    r = ref(f)
+    print type(r())
+    # del f
+    f = 1
+    assert not r()
+for i in xrange(40):
+    f1()
+
+def f3():
+    class MyIter(object):
+        def __init__(self):
+            self.n = 5
+        def __del__(self):
+            print "deleting iter"
+        def next(self):
+            if self.n:
+                self.n -= 1
+                return self.n
+            raise StopIteration()
+
+    class MyIterable(object):
+        def __iter__(self):
+            return MyIter()
+
+    for i in MyIterable():
+        print i
+    else:
+        print -1
+f3()

test/tests/resurrection.py

-# expected: fail
-#  - finalization (let alone resurrection) not implemented yet
-
 # Objects are allowed to resurrect themselves in their __del__ methods...
-# Note: the behavior here will differ from cPython and maybe PyPy
 
 x = None
 running = True