Fix up a bunch of problems with definedness analysis

Not sure how these didn't show up before...

src/analysis/fpc.h

@@ -59,7 +59,7 @@ typename BBAnalyzer<T>::AllMap computeFixedPoint(CFG* cfg, const BBAnalyzer<T>&
         CFGBlock* block = q.top();
         q.pop();
 
-        Map initial = states[block];
+        Map& initial = states[block];
         if (VERBOSITY("analysis") >= 2)
             printf("fpc on block %d - %ld entries\n", block->idx, initial.size());
 

src/analysis/function_analysis.cpp

@@ -292,19 +292,19 @@ DefinednessAnalysis::DefinednessAnalysis(const SourceInfo::ArgNames& arg_names,
             // printf("%d %s %d\n", p.first->idx, p2.first.c_str(), p2.second);
             required.insert(p2.first);
         }
-        defined.insert(make_pair(p.first, required));
+        defined_at_end.insert(make_pair(p.first, required));
     }
 }
 
-DefinednessAnalysis::DefinitionLevel DefinednessAnalysis::isDefinedAt(const std::string& name, CFGBlock* block) {
+DefinednessAnalysis::DefinitionLevel DefinednessAnalysis::isDefinedAtEnd(const std::string& name, CFGBlock* block) {
     std::unordered_map<std::string, DefinitionLevel>& map = results[block];
     if (map.count(name) == 0)
         return Undefined;
     return map[name];
 }
 
-const DefinednessAnalysis::RequiredSet& DefinednessAnalysis::getDefinedNamesAt(CFGBlock* block) {
-    return defined[block];
+const DefinednessAnalysis::RequiredSet& DefinednessAnalysis::getDefinedNamesAtEnd(CFGBlock* block) {
+    return defined_at_end[block];
 }
 
 PhiAnalysis::PhiAnalysis(const SourceInfo::ArgNames& arg_names, CFG* cfg, LivenessAnalysis* liveness,
@@ -313,19 +313,21 @@ PhiAnalysis::PhiAnalysis(const SourceInfo::ArgNames& arg_names, CFG* cfg, Livene
 
     for (CFGBlock* block : cfg->blocks) {
         RequiredSet required;
-        if (block->predecessors.size() < 2)
-            continue;
 
-        const RequiredSet& defined = definedness.getDefinedNamesAt(block);
-        if (defined.size())
-            assert(block->predecessors.size());
+        if (block->predecessors.size() > 1) {
+            for (CFGBlock* pred : block->predecessors) {
+                const RequiredSet& defined = definedness.getDefinedNamesAtEnd(pred);
                 for (const auto& s : defined) {
-            if (liveness->isLiveAtEnd(s, block->predecessors[0])) {
+                    if (required.count(s) == 0 && liveness->isLiveAtEnd(s, pred)) {
+                        // printf("%d-%d %s\n", pred->idx, block->idx, s.c_str());
+
                         required.insert(s);
                     }
                 }
+            }
+        }
 
-        required_phis.insert(make_pair(block, required));
+        required_phis.insert(make_pair(block, std::move(required)));
     }
 }
 
@@ -336,8 +338,8 @@ const PhiAnalysis::RequiredSet& PhiAnalysis::getAllRequiredAfter(CFGBlock* block
     return required_phis[block->successors[0]];
 }
 
-const PhiAnalysis::RequiredSet& PhiAnalysis::getAllDefinedAt(CFGBlock* block) {
-    return definedness.getDefinedNamesAt(block);
+const PhiAnalysis::RequiredSet& PhiAnalysis::getAllRequiredFor(CFGBlock* block) {
+    return required_phis[block];
 }
 
 bool PhiAnalysis::isRequired(const std::string& name, CFGBlock* block) {
@@ -358,11 +360,16 @@ bool PhiAnalysis::isRequiredAfter(const std::string& name, CFGBlock* block) {
 
 bool PhiAnalysis::isPotentiallyUndefinedAfter(const std::string& name, CFGBlock* block) {
     assert(!startswith(name, "!"));
-    assert(block->successors.size() > 0);
-    DefinednessAnalysis::DefinitionLevel dlevel = definedness.isDefinedAt(name, block->successors[0]);
-    ASSERT(dlevel != DefinednessAnalysis::Undefined, "%s %d", name.c_str(), block->idx);
 
-    return dlevel == DefinednessAnalysis::PotentiallyDefined;
+    if (block->successors.size() != 1)
+        return false;
+
+    for (CFGBlock* pred : block->successors[0]->predecessors) {
+        DefinednessAnalysis::DefinitionLevel dlevel = definedness.isDefinedAtEnd(name, pred);
+        if (dlevel != DefinednessAnalysis::Defined)
+            return true;
+    }
+    return false;
 }
 
 LivenessAnalysis* computeLivenessInfo(CFG*) {

src/analysis/function_analysis.h

@@ -49,14 +49,14 @@ public:
 
 private:
     std::unordered_map<CFGBlock*, std::unordered_map<std::string, DefinitionLevel> > results;
-    std::unordered_map<CFGBlock*, const RequiredSet> defined;
+    std::unordered_map<CFGBlock*, const RequiredSet> defined_at_end;
     ScopeInfo* scope_info;
 
 public:
     DefinednessAnalysis(const SourceInfo::ArgNames& args, CFG* cfg, ScopeInfo* scope_info);
 
-    DefinitionLevel isDefinedAt(const std::string& name, CFGBlock* block);
-    const RequiredSet& getDefinedNamesAt(CFGBlock* block);
+    DefinitionLevel isDefinedAtEnd(const std::string& name, CFGBlock* block);
+    const RequiredSet& getDefinedNamesAtEnd(CFGBlock* block);
 };
 class PhiAnalysis {
 public:
@@ -73,7 +73,7 @@ public:
     bool isRequired(const std::string& name, CFGBlock* block);
     bool isRequiredAfter(const std::string& name, CFGBlock* block);
     const RequiredSet& getAllRequiredAfter(CFGBlock* block);
-    const RequiredSet& getAllDefinedAt(CFGBlock* block);
+    const RequiredSet& getAllRequiredFor(CFGBlock* block);
     bool isPotentiallyUndefinedAfter(const std::string& name, CFGBlock* block);
 };
 

src/codegen/compvars.cpp

@@ -1195,6 +1195,10 @@ public:
     virtual ConcreteCompilerType* getConcreteType() { return this; }
     // Shouldn't call this:
     virtual ConcreteCompilerType* getBoxType() { RELEASE_ASSERT(0, ""); }
+
+    void drop(IREmitter& emitter, VAR* var) override {}
+    void grab(IREmitter& emitter, VAR* var) override {}
+
 } _CLOSURE;
 ConcreteCompilerType* CLOSURE = &_CLOSURE;
 
@@ -1263,6 +1267,8 @@ public:
 
         if (rtn == NULL) {
             rtn = new VAR(this, var->getValue(), var->isGrabbed());
+            while (rtn->getVrefs() < var->getVrefs())
+                rtn->incvref();
         }
         return rtn;
     }

src/codegen/irgen.cpp

@@ -604,12 +604,8 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList& out_gua
                 into_hax.insert(b2);
             }
 
-            const PhiAnalysis::RequiredSet& names = source->phis->getAllDefinedAt(block);
+            const PhiAnalysis::RequiredSet& names = source->phis->getAllRequiredFor(block);
             for (const auto& s : names) {
-                // TODO the list from getAllDefinedAt should come filtered:
-                if (!source->liveness->isLiveAtEnd(s, block->predecessors[0]))
-                    continue;
-
                 // printf("adding guessed phi for %s\n", s.c_str());
                 ConcreteCompilerType* type = types->getTypeAtBlockStart(s, block);
                 llvm::PHINode* phi = emitter->getBuilder()->CreatePHI(type->llvmType(), block->predecessors.size(), s);

src/codegen/irgen/irgenerator.cpp

@@ -731,7 +731,7 @@ private:
                 std::vector<CompilerVariable*> args;
                 args.push_back(key);
                 args.push_back(value);
-                // TODO could use the internal _listAppend function to avoid incref/decref'ing None
+                // TODO should use callattr
                 CompilerVariable* rtn = setitem->call(emitter, getEmptyOpInfo(exc_info), ArgPassSpec(2), args, NULL);
                 rtn->decvref(emitter);
 
@@ -1245,30 +1245,29 @@ private:
         snprintf(buf, 40, "!%s_%s", prefix, token);
         return std::string(buf);
     }
+
     void _setFake(std::string name, CompilerVariable* val) {
         assert(name[0] == '!');
         CompilerVariable*& cur = symbol_table[name];
         assert(cur == NULL);
         cur = val;
     }
-    CompilerVariable* _clearFake(std::string name) {
-        assert(name[0] == '!');
-        CompilerVariable* rtn = symbol_table[name];
-        assert(rtn == NULL);
-        symbol_table.erase(name);
-        return rtn;
-    }
 
     CompilerVariable* _getFake(std::string name, bool allow_missing = false) {
         assert(name[0] == '!');
-        CompilerVariable* rtn = symbol_table[name];
-        if (!allow_missing)
-            assert(rtn != NULL);
-        return rtn;
+        auto it = symbol_table.find(name);
+        if (it == symbol_table.end()) {
+            assert(allow_missing);
+            return NULL;
         }
+        return it->second;
+    }
+
     CompilerVariable* _popFake(std::string name, bool allow_missing = false) {
         CompilerVariable* rtn = _getFake(name, allow_missing);
         symbol_table.erase(name);
+        if (!allow_missing)
+            assert(rtn != NULL);
         return rtn;
     }
 
@@ -1670,6 +1669,11 @@ private:
         rtn->ensureGrabbed(emitter);
         val->decvref(emitter);
 
+        for (auto& p : symbol_table) {
+            p.second->decvref(emitter);
+        }
+        symbol_table.clear();
+
         endBlock(DEAD);
 
         ASSERT(rtn->getVrefs() == 1, "%d", rtn->getVrefs());
@@ -1747,18 +1751,6 @@ private:
         std::vector<ConcreteCompilerVariable*> converted_args;
 
         SortedSymbolTable sorted_symbol_table(symbol_table.begin(), symbol_table.end());
-        /*
-        for (SortedSymbolTable::iterator it = sorted_symbol_table.begin(), end = sorted_symbol_table.end(); it != end; )
-        {
-            if (!source->liveness->isLiveAtEnd(it->first, myblock)) {
-                // I think this line can never get hit: nothing can die on a backedge, since control flow can eventually
-                // reach this block again, where the symbol was live (as shown by it being in the symbol table)
-                printf("Not sending %s to osr since it will die\n", it->first.c_str());
-                it = sorted_symbol_table.erase(it);
-            } else {
-                ++it;
-            }
-        }*/
 
         // For OSR calls, we use the same calling convention as in some other places; namely,
         // arg1, arg2, arg3, argarray [nargs is ommitted]
@@ -1792,11 +1784,6 @@ private:
         int arg_num = -1;
         for (const auto& p : sorted_symbol_table) {
             arg_num++;
-            // I don't think this can fail, but if it can we should filter out dead symbols before
-            // passing them on:
-            ASSERT(startswith(p.first, "!is_defined")
-                   || irstate->getSourceInfo()->liveness->isLiveAtEnd(p.first, myblock),
-                   "%d %s", myblock->idx, p.first.c_str());
 
             // This line can never get hit right now since we unnecessarily force every variable to be concrete
             // for a loop, since we generate all potential phis:
@@ -1828,6 +1815,12 @@ private:
                 } else if (var->getType() == FLOAT) {
                     // val = emitter.getBuilder()->CreateBitCast(val, g.llvm_value_type_ptr);
                     ptr = emitter.getBuilder()->CreateBitCast(ptr, g.double_->getPointerTo());
+                } else if (var->getType() == UNDEF) {
+                    // TODO if there are any undef variables, we're in 'unreachable' territory.
+                    // Do we even need to generate any of this code?
+
+                    // Currently we represent 'undef's as 'i16 undef'
+                    val = emitter.getBuilder()->CreateIntToPtr(val, g.llvm_value_type_ptr);
                 } else {
                     assert(val->getType() == g.llvm_value_type_ptr);
                 }
@@ -2040,11 +2033,14 @@ private:
                 ++it;
             } else {
 #ifndef NDEBUG
-                // TODO getTypeAtBlockEnd will automatically convert up to the concrete type, which we don't want here,
-                // but this is just for debugging so I guess let it happen for now:
+                if (myblock->successors.size()) {
+                    // TODO getTypeAtBlockEnd will automatically convert up to the concrete type, which we don't want
+                    // here, but this is just for debugging so I guess let it happen for now:
                     ConcreteCompilerType* ending_type = types->getTypeAtBlockEnd(it->first, myblock);
                     ASSERT(it->second->canConvertTo(ending_type), "%s is supposed to be %s, but somehow is %s",
-                       it->first.c_str(), ending_type->debugName().c_str(), it->second->getType()->debugName().c_str());
+                           it->first.c_str(), ending_type->debugName().c_str(),
+                           it->second->getType()->debugName().c_str());
+                }
 #endif
 
                 ++it;
@@ -2099,19 +2095,14 @@ public:
 
         SymbolTable* st = new SymbolTable(symbol_table);
         ConcreteSymbolTable* phi_st = new ConcreteSymbolTable();
-        for (SymbolTable::iterator it = st->begin(); it != st->end(); it++) {
-            if (it->first[0] == '!') {
-                // ASSERT(startswith(it->first, _getFakeName("is_defined", "")), "left a fake variable in the real
-                // symbol table? '%s'", it->first.c_str());
-            } else {
-                ASSERT(source->liveness->isLiveAtEnd(it->first, myblock), "%s", it->first.c_str());
-            }
-        }
 
         if (myblock->successors.size() == 0) {
-            st->erase(CREATED_CLOSURE_NAME);
-            st->erase(PASSED_CLOSURE_NAME);
-            assert(st->size() == 0); // shouldn't have anything live if there are no successors!
+            for (auto& p : *st) {
+                p.second->decvref(emitter);
+            }
+            st->clear();
+            symbol_table.clear();
+
             return EndingState(st, phi_st, curblock);
         } else if (myblock->successors.size() > 1) {
             // Since there are no critical edges, all successors come directly from this node,

src/core/cfg.cpp

@@ -379,6 +379,8 @@ private:
     AST_expr* remapAttribute(AST_Attribute* node) {
         AST_Attribute* rtn = new AST_Attribute();
 
+        rtn->col_offset = node->col_offset;
+        rtn->lineno = node->lineno;
         rtn->ctx_type = node->ctx_type;
         rtn->attr = node->attr;
         rtn->value = remapExpr(node->value);
@@ -483,6 +485,8 @@ private:
     AST_expr* remapClsAttribute(AST_ClsAttribute* node) {
         AST_ClsAttribute* rtn = new AST_ClsAttribute();
 
+        rtn->col_offset = node->col_offset;
+        rtn->lineno = node->lineno;
         rtn->attr = node->attr;
         rtn->value = remapExpr(node->value);
         return rtn;