irgen fix: unbox+reboxing doesn't maintain object id

and sqlalchemy needs it to.  Disable unboxing for now; I think
I know how to reenable it but I don't think there will be much
of a perf impact.

src/analysis/type_analysis.cpp

@@ -56,13 +56,16 @@ ConcreteCompilerType* NullTypeAnalysis::getTypeAtBlockEnd(InternedString name, C
 }
 
 
+// Note: the behavior of this function must match irgenerator.cpp::unboxVar()
 static ConcreteCompilerType* unboxedType(ConcreteCompilerType* t) {
+    if (t == BOXED_BOOL)
+        return BOOL;
+#if ENABLE_UNBOXED_VALUES
     if (t == BOXED_INT)
         return INT;
     if (t == BOXED_FLOAT)
         return FLOAT;
-    if (t == BOXED_BOOL)
-        return BOOL;
+#endif
     return t;
 }
 

src/codegen/compvars.cpp

@@ -1778,6 +1778,7 @@ public:
             llvm::Value* unboxed = emitter.getBuilder()->CreateCall(g.funcs.unboxBool, rtn->getValue());
             return boolFromI1(emitter, unboxed);
         }
+#if ENABLE_UNBOXED_VALUES
         if (cf->spec->rtn_type == BOXED_INT) {
             llvm::Value* unboxed = emitter.getBuilder()->CreateCall(g.funcs.unboxInt, rtn->getValue());
             return new ConcreteCompilerVariable(INT, unboxed, true);
@@ -1787,8 +1788,9 @@ public:
             return new ConcreteCompilerVariable(FLOAT, unboxed, true);
         }
         assert(cf->spec->rtn_type != BOXED_INT);
-        ASSERT(cf->spec->rtn_type != BOXED_BOOL, "%p", cf->code);
         assert(cf->spec->rtn_type != BOXED_FLOAT);
+#endif
+        ASSERT(cf->spec->rtn_type != BOXED_BOOL, "%p", cf->code);
 
         return rtn;
     }

src/codegen/irgen/irgenerator.cpp

@@ -1511,7 +1511,9 @@ private:
         return func;
     }
 
+    // Note: the behavior of this function must match type_analysis.cpp:unboxedType()
     ConcreteCompilerVariable* unboxVar(ConcreteCompilerType* t, llvm::Value* v, bool grabbed) {
+#if ENABLE_UNBOXED_VALUES
         if (t == BOXED_INT) {
             llvm::Value* unboxed = emitter.getBuilder()->CreateCall(g.funcs.unboxInt, v);
             ConcreteCompilerVariable* rtn = new ConcreteCompilerVariable(INT, unboxed, true);
@@ -1522,6 +1524,7 @@ private:
             ConcreteCompilerVariable* rtn = new ConcreteCompilerVariable(FLOAT, unboxed, true);
             return rtn;
         }
+#endif
         if (t == BOXED_BOOL) {
             llvm::Value* unboxed = emitter.getBuilder()->CreateCall(g.funcs.unboxBool, v);
             return boolFromI1(emitter, unboxed);
@@ -2199,9 +2202,11 @@ private:
             ConcreteCompilerVariable* var = p.second->makeConverted(emitter, p.second->getConcreteType());
             converted_args.push_back(var);
 
+#if ENABLE_UNBOXED_VALUES
             assert(var->getType() != BOXED_INT && "should probably unbox it, but why is it boxed in the first place?");
             assert(var->getType() != BOXED_FLOAT
                    && "should probably unbox it, but why is it boxed in the first place?");
+#endif
 
             // This line can never get hit right now for the same reason that the variables must already be
             // concrete,
@@ -2648,8 +2653,10 @@ public:
         assert(name.s() != FRAME_INFO_PTR_NAME);
         ASSERT(irstate->getScopeInfo()->getScopeTypeOfName(name) != ScopeInfo::VarScopeType::GLOBAL, "%s",
                name.c_str());
+#if ENABLE_UNBOXED_VALUES
         assert(var->getType() != BOXED_INT);
         assert(var->getType() != BOXED_FLOAT);
+#endif
         CompilerVariable*& cur = symbol_table[name];
         assert(cur == NULL);
         cur = var;

src/core/options.h

@@ -51,6 +51,12 @@ extern bool ENABLE_ICS, ENABLE_ICGENERICS, ENABLE_ICGETITEMS, ENABLE_ICSETITEMS,
 extern bool BOOLS_AS_I64;
 
 #define ENABLE_SAMPLING_PROFILER 0
+
+// Our current implementation of unbox values has some minor compatibility issues, where it can
+// change the apparent id() / is-equality of a boxed value (by inserting extra unbox+box pairs).
+// I think it can be rescued (we need the unboxed compilertype to remember the boxed value),
+// but for now it's just turned off with this flag.
+#define ENABLE_UNBOXED_VALUES 0
 }
 }
 

test/tests/int_ids.py

+# Regression test:
+# speculation should not result in ints changing their ids.
+# I guess this means that if we unbox, we have to know what we would box to.
+
+class C(object):
+    pass
+
+c = C()
+c.a = 100000
+
+# Test it via osr:
+def f():
+    for i in xrange(11000):
+        a1 = c.a
+        a2 = c.a
+        if 0:
+            pass
+        assert id(a1) == id(a2), i
+f()
+
+# Test it via reopt:
+def f2():
+    a1 = c.a
+    a2 = c.a
+    if 0:
+        pass
+    assert id(a1) == id(a2)
+for i in xrange(11000):
+    f2()
+
+# Test function returns:
+def g():
+    return 1000
+def f3():
+    assert id(g()) == id(g())
+for i in xrange(11000):
+    f3()
+
+# Test function args:
+def f4(a, b):
+    assert id(a) == id(b)
+for i in xrange(11000):
+    f4(1000, 1000)