Fix the propagation of is-defined-ness through an OSR, though that exposes a number of other issues

src/analysis/function_analysis.cpp

@@ -133,6 +133,7 @@ class DefinednessBBAnalyzer : public BBAnalyzer<DefinednessAnalysis::DefinitionL
             return DefinednessAnalysis::PotentiallyDefined;
         }
 };
+
 class DefinednessVisitor : public ASTVisitor {
     private:
         typedef DefinednessBBAnalyzer::Map Map;
@@ -218,6 +219,7 @@ class DefinednessVisitor : public ASTVisitor {
             return true;
         }
 };
+
 void DefinednessBBAnalyzer::processBB(Map &starting, CFGBlock *block) const {
     DefinednessVisitor visitor(starting);
     for (int i = 0; i < block->body.size(); i++) {
@@ -226,6 +228,13 @@ void DefinednessBBAnalyzer::processBB(Map &starting, CFGBlock *block) const {
     if (block->idx == 0 && arguments) {
         arguments->accept(&visitor);
     }
+
+    if (VERBOSITY("analysis") >= 2) {
+        printf("At end of block %d:\n", block->idx);
+        for (auto p : starting) {
+            printf("%s: %d\n", p.first.c_str(), p.second);
+        }
+    }
 }
 
 DefinednessAnalysis::DefinednessAnalysis(AST_arguments *args, CFG* cfg, ScopeInfo *scope_info) : scope_info(scope_info) {
@@ -291,10 +300,12 @@ const PhiAnalysis::RequiredSet& PhiAnalysis::getAllDefinedAt(CFGBlock *block) {
 }
 
 bool PhiAnalysis::isRequired(const std::string &name, CFGBlock* block) {
+    assert(!startswith(name, "!"));
     return required_phis[block].count(name) != 0;
 }
 
 bool PhiAnalysis::isRequiredAfter(const std::string &name, CFGBlock* block) {
+    assert(!startswith(name, "!"));
     // If there are multiple successors, then none of them are allowed
     // to require any phi nodes
     if (block->successors.size() != 1)
@@ -305,6 +316,7 @@ 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);

src/codegen/irgen.cpp

@@ -342,7 +342,11 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
                 assert(from_arg->getType() == it->second->llvmType());
             }
 
-            ConcreteCompilerType *phi_type = types->getTypeAtBlockStart(it->first, target_block);
+            ConcreteCompilerType *phi_type;
+            if (startswith(it->first, "!is_defined"))
+                phi_type = BOOL;
+            else
+                phi_type = types->getTypeAtBlockStart(it->first, target_block);
             //ConcreteCompilerType *analyzed_type = types->getTypeAtBlockStart(it->first, block);
             //ConcreteCompilerType *phi_type = (*phis)[it->first].first;
 
@@ -401,6 +405,11 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
         }
 
         if (guard_val) {
+            // Create the guard with both branches leading to the success_bb,
+            // and let the deopt path change the failure case to point to the
+            // as-yet-unknown deopt block.
+            // TODO Not the best approach since if we fail to do that patching,
+            // the guard will just silently be ignored.
             llvm::BranchInst *br = entry_emitter->getBuilder()->CreateCondBr(guard_val, osr_unbox_block, osr_unbox_block);
             out_guards.registerGuardForBlockEntry(target_block, br, *initial_syms);
         } else {
@@ -541,7 +550,12 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
             assert(phis);
 
             for (OSREntryDescriptor::ArgMap::const_iterator it = entry_descriptor->args.begin(), end = entry_descriptor->args.end(); it != end; ++it) {
-                ConcreteCompilerType *analyzed_type = types->getTypeAtBlockStart(it->first, block);
+                ConcreteCompilerType *analyzed_type;
+                if (startswith(it->first, "!is_defined"))
+                    analyzed_type = BOOL;
+                else
+                    analyzed_type = types->getTypeAtBlockStart(it->first, block);
+
                 //printf("For %s, given %s, analyzed for %s\n", it->first.c_str(), it->second->debugName().c_str(), analyzed_type->debugName().c_str());
 
                 llvm::PHINode *phi = emitter->getBuilder()->CreatePHI(analyzed_type->llvmType(), block->predecessors.size()+1, it->first);
@@ -580,7 +594,7 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
             if (block->predecessors.size() == 1)  {
                 // If this block has only one predecessor, it by definition doesn't need any phi nodes.
                 // Assert that the phi_st is empty, and just create the symbol table from the non-phi st:
-                assert(phi_ending_symbol_tables[pred->idx]->size() == 0);
+                ASSERT(phi_ending_symbol_tables[pred->idx]->size() == 0, "%d %d", block->idx, pred->idx);
                 assert(ending_symbol_tables.count(pred->idx));
                 generator->copySymbolsFrom(ending_symbol_tables[pred->idx]);
             } else {
@@ -621,7 +635,7 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
     // We don't know the exact ssa values to back-propagate to the phi nodes until we've generated
     // the relevant IR, so after we have done all of it, go back through and populate the phi nodes.
     // Also, do some checking to make sure that the phi analysis stuff worked out, and that all blocks
-    // agreed on what symbols + types they should be propagiting for the phis.
+    // agreed on what symbols + types they should be propagating for the phis.
     for (int i = 0; i < source->cfg->blocks.size(); i++) {
         PHITable *phis = created_phis[i];
         if (phis == NULL)
@@ -632,6 +646,7 @@ static void emitBBs(IRGenState* irstate, const char* bb_type, GuardList &out_gua
         CFGBlock *b = source->cfg->blocks[i];
 
         const std::vector<GuardList::BlockEntryGuard*> &block_guards = in_guards.getGuardsForBlock(b);
+        //printf("Found %ld guards for block %p, for %p\n", block_guards.size(), b, &in_guards);
 
         for (int j = 0; j < b->predecessors.size(); j++) {
             CFGBlock *b2 = b->predecessors[j];
@@ -900,18 +915,24 @@ CompiledFunction* compileFunction(SourceInfo *source, const OSREntryDescriptor *
         GuardList deopt_guards;
         //typedef std::unordered_map<CFGBlock*, std::unordered_map<AST_expr*, GuardList::ExprTypeGuard*> > Worklist;
         //Worklist guard_worklist;
+
+        guards.getBlocksWithGuards(deopt_full_blocks);
         for (GuardList::expr_type_guard_iterator it = guards.after_begin(), end = guards.after_end(); it != end; ++it) {
             deopt_partial_blocks.insert(it->second->cfg_block);
         }
 
         computeBlockSetClosure(deopt_full_blocks, deopt_partial_blocks);
 
+        assert(deopt_full_blocks.size() || deopt_partial_blocks.size());
+
         TypeAnalysis *deopt_types = doTypeAnalysis(source->cfg, arg_names, sig->arg_types, TypeAnalysis::NONE, source->scoping->getScopeInfoForNode(source->ast));
         emitBBs(&irstate, "deopt", deopt_guards, guards, deopt_types, arg_names, NULL, deopt_full_blocks, deopt_partial_blocks);
         assert(deopt_guards.isEmpty());
+        deopt_guards.assertGotPatched();
 
         delete deopt_types;
     }
+    guards.assertGotPatched();
 
     for (GuardList::expr_type_guard_iterator it = guards.after_begin(), end = guards.after_end(); it != end; ++it) {
         delete it->second;

src/codegen/irgen/irgenerator.cpp

@@ -203,6 +203,11 @@ class IRGeneratorImpl : public IRGenerator {
             llvm::BasicBlock* success_bb = llvm::BasicBlock::Create(g.context, "check_succeeded", irstate->getLLVMFunction());
             success_bb->moveAfter(curblock);
 
+            // Create the guard with both branches leading to the success_bb,
+            // and let the deopt path change the failure case to point to the
+            // as-yet-unknown deopt block.
+            // TODO Not the best approach since if we fail to do that patching,
+            // the guard will just silently be ignored.
             llvm::BranchInst* guard = emitter.getBuilder()->CreateCondBr(check_val, success_bb, success_bb, branch_weights);
 
             curblock = success_bb;
@@ -1347,7 +1352,7 @@ class IRGeneratorImpl : public IRGenerator {
             for (SortedSymbolTable::iterator it = sorted_symbol_table.begin(), end = sorted_symbol_table.end(); it != end; ++it, ++i) {
                 // I don't think this can fail, but if it can we should filter out dead symbols before
                 // passing them on:
-                ASSERT(irstate->getSourceInfo()->liveness->isLiveAtEnd(it->first, myblock), "%d %s", myblock->idx, it->first.c_str());
+                ASSERT(startswith(it->first, "!is_defined") ||  irstate->getSourceInfo()->liveness->isLiveAtEnd(it->first, myblock), "%d %s", myblock->idx, it->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:
@@ -1500,6 +1505,11 @@ class IRGeneratorImpl : public IRGenerator {
             ScopeInfo *scope_info = irstate->getScopeInfo();
 
             for (SymbolTable::iterator it = symbol_table.begin(); it != symbol_table.end();) {
+                if (startswith(it->first, "!is_defined")) {
+                    ++it;
+                    continue;
+                }
+
                 //ASSERT(it->first[0] != '!' || startswith(it->first, "!is_defined"), "left a fake variable in the real symbol table? '%s'", it->first.c_str());
 
                 if (!source->liveness->isLiveAtEnd(it->first, myblock)) {
@@ -1514,7 +1524,7 @@ class IRGeneratorImpl : public IRGenerator {
                     ConcreteCompilerVariable *v = it->second->makeConverted(emitter, phi_type);
                     it->second->decvref(emitter);
                     symbol_table[it->first] = v->split(emitter);
-                    it++;
+                    ++it;
                 } else {
 #ifndef NDEBUG
                     // TODO getTypeAtBlockEnd will automatically convert up to the concrete type, which we don't want here,
@@ -1523,7 +1533,7 @@ class IRGeneratorImpl : public IRGenerator {
                     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());
 #endif
 
-                    it++;
+                    ++it;
                 }
             }
 
@@ -1533,18 +1543,29 @@ class IRGeneratorImpl : public IRGenerator {
                 assert(!scope_info->refersToGlobal(*it));
                 CompilerVariable* &cur = symbol_table[*it];
 
+                std::string defined_name = _getFakeName("is_defined", it->c_str());
+
                 if (cur != NULL) {
+                    //printf("defined on this path; ");
+
+                    ConcreteCompilerVariable *is_defined = static_cast<ConcreteCompilerVariable*>(_getFake(defined_name, true));
+
                     if (source->phis->isPotentiallyUndefinedAfter(*it, myblock)) {
-                        //printf("is potentially undefined\n");
-                        _setFake(_getFakeName("is_defined", it->c_str()), new ConcreteCompilerVariable(BOOL, getConstantInt(1, g.i1), true));
+                        //printf("is potentially undefined later, so marking it defined\n");
+                        if (is_defined) {
+                            _setFake(defined_name, is_defined);
+                        } else {
+                            _setFake(defined_name, new ConcreteCompilerVariable(BOOL, getConstantInt(1, g.i1), true));
+                        }
                     } else {
-                        //printf("is definitely defined\n");
+                        //printf("is defined in all later paths, so not marking\n");
+                        assert(!is_defined);
                     }
                 } else {
                     //printf("no st entry, setting undefined\n");
                     ConcreteCompilerType *phi_type = types->getTypeAtBlockEnd(*it, myblock);
                     cur = new ConcreteCompilerVariable(phi_type, llvm::UndefValue::get(phi_type->llvmType()), true);
-                    _setFake(_getFakeName("is_defined", it->c_str()), new ConcreteCompilerVariable(BOOL, getConstantInt(0, g.i1), true));
+                    _setFake(defined_name, new ConcreteCompilerVariable(BOOL, getConstantInt(0, g.i1), true));
                 }
             }
 
@@ -1581,6 +1602,15 @@ class IRGeneratorImpl : public IRGenerator {
             }
 
             assert(myblock->successors.size() == 1); // other cases should have been handled
+
+            // In theory this case shouldn't be necessary:
+            if (myblock->successors[0]->predecessors.size() == 1) {
+                // If the next block has a single predecessor, don't have to
+                // emit any phis.
+                // Should probably not emit no-op jumps like this though.
+                return EndingState(st, phi_st, curblock);
+            }
+
             for (SymbolTable::iterator it = st->begin(); it != st->end();) {
                 if (startswith(it->first, "!is_defined") || source->phis->isRequiredAfter(it->first, myblock)) {
                     assert(it->second->isGrabbed());

src/codegen/irgen/irgenerator.h

@@ -124,6 +124,27 @@ class GuardList {
         expr_type_guard_iterator after_end() {
             return expr_type_guards.end();
         }
+
+        void getBlocksWithGuards(std::unordered_set<CFGBlock*> &add_to) {
+            for (auto p : block_begin_guards) {
+                add_to.insert(p.first);
+            }
+        }
+
+        void assertGotPatched() {
+#ifndef NDEBUG
+            for (auto p : block_begin_guards) {
+                for (auto g : p.second) {
+                    assert(g->branch->getSuccessor(0) != g->branch->getSuccessor(1));
+                }
+            }
+
+            for (auto p : expr_type_guards) {
+                assert(p.second->branch->getSuccessor(0) != p.second->branch->getSuccessor(1));
+            }
+#endif
+        }
+
         ExprTypeGuard* getNodeTypeGuard(AST_expr* node) const {
             expr_type_guard_const_iterator it = expr_type_guards.find(node);
             if (it == expr_type_guards.end())
@@ -132,7 +153,7 @@ class GuardList {
         }
 
         bool isEmpty() const {
-            return expr_type_guards.size() == 0;
+            return expr_type_guards.size() == 0 && block_begin_guards.size() == 0;
         }
 
         void addExprTypeGuard(CFGBlock *cfg_block, llvm::BranchInst* branch, AST_expr* ast_node, CompilerVariable* val, const SymbolTable &st) {
@@ -142,6 +163,7 @@ class GuardList {
         }
 
         void registerGuardForBlockEntry(CFGBlock *cfg_block, llvm::BranchInst* branch, const SymbolTable &st) {
+            //printf("Adding guard for block %p, in %p\n", cfg_block, this);
             std::vector<BlockEntryGuard*> &v = block_begin_guards[cfg_block];
             v.push_back(new BlockEntryGuard(cfg_block, branch, st));
         }

src/codegen/llvm_interpreter.cpp

@@ -174,6 +174,11 @@ Val fetch(llvm::Value* v, const llvm::DataLayout &dl, const SymMap &symbols) {
             RELEASE_ASSERT(0, "");
         }
         case llvm::Value::UndefValueVal:
+            // It's ok to evaluate an undef as long as we're being careful
+            // to not use it later.
+            // Typically this happens if we need to propagate the 'value' of an
+            // maybe-defined Python variable; we won't actually read from it if
+            // it's undef, since it should be guarded by an !is_defined variable.
             return (int64_t)-1337;
         default:
             v->dump();

src/core/cfg.cpp

@@ -200,13 +200,13 @@ class CFGVisitor : public ASTVisitor {
 
         std::string nodeName(AST_expr* node) {
             char buf[40];
-            snprintf(buf, 40, "!%p", node);
+            snprintf(buf, 40, "#%p", node);
             return std::string(buf);
         }
 
         std::string nodeName(AST_expr* node, const std::string &suffix, int idx) {
             char buf[50];
-            snprintf(buf, 50, "!%p_%s_%d", node, suffix.c_str(), idx);
+            snprintf(buf, 50, "#%p_%s_%d", node, suffix.c_str(), idx);
             return std::string(buf);
         }
 

test/tests/osr_into_failed_speculation.py

+# expected: fail
+
+# Try to trick the JIT into OSR'ing into an optimized version with a speculation
+# that has already failed.
+# In the f() function, y will have type int, but when we OSR from the while loop,
+# we'll calculate the types as if xrange() had returned an xrange object.  We'll
+# have to emit a guard for that and branch to a deopt version.
+
+def xrange(n):
+    return n
+
+def f(x):
+    y = xrange(x)
+
+    n = 100000
+    while n:
+        n -= 1
+
+    print y
+    print y + 1
+
+f(11)
+f(10)

test/tests/osr_maybe_undefined.py

@@ -4,12 +4,13 @@
 # is potentially-undefined.
 
 def f(x):
-    if x:
-        y = 1
+    if x % 2:
+        y = x
 
-    for i in xrange(10000000):
+    for i in xrange(1000000):
         pass
 
     print y
-f(1)
-f(0)
+
+f(11)
+f(10)

test/tests/osr_maybe_undefined2.py

+# expected: fail
+
+# Regression test to make sure we can do an OSR if one of the live variables
+# is potentially-undefined.
+
+def f(x):
+    if x % 2:
+        y = x
+
+    xrange(0)
+
+    for i in xrange(1000000):
+        xrange(0)
+
+    print y
+    xrange(0)
+
+f(11)
+f(10)