Bug 824257 - Remove type barriers that are redundant with null/undefined checks, r=jandem.

js/src/ion/Ion.cpp

@@ -956,11 +956,11 @@ CompileBackEnd(MIRGenerator *mir)
             return NULL;
     }
 
-    // Note: bounds check elimination has to run after all other passes that
-    // move instructions. Since bounds check uses are replaced with the actual
-    // index, code motion after this pass could incorrectly move a load or
-    // store before its bounds check.
-    if (!EliminateRedundantBoundsChecks(graph))
+    // Note: check elimination has to run after all other passes that move
+    // instructions. Since check uses are replaced with the actual index, code
+    // motion after this pass could incorrectly move a load or store before its
+    // bounds check.
+    if (!EliminateRedundantChecks(graph))
         return NULL;
     IonSpewPass("Bounds Check Elimination");
     AssertGraphCoherency(graph);

js/src/ion/IonAnalysis.cpp

@@ -1134,10 +1134,29 @@ ion::ExtractLinearInequality(MTest *test, BranchDirection direction,
 }
 
 static bool
-TryEliminateBoundsCheck(MBoundsCheck *dominating, MBoundsCheck *dominated, bool *eliminated)
+TryEliminateBoundsCheck(BoundsCheckMap &checks, size_t blockIndex, MBoundsCheck *dominated, bool *eliminated)
 {
     JS_ASSERT(!*eliminated);
 
+    // Replace all uses of the bounds check with the actual index.
+    // This is (a) necessary, because we can coalesce two different
+    // bounds checks and would otherwise use the wrong index and
+    // (b) helps register allocation. Note that this is safe since
+    // no other pass after bounds check elimination moves instructions.
+    dominated->replaceAllUsesWith(dominated->index());
+
+    if (!dominated->isMovable())
+        return true;
+
+    MBoundsCheck *dominating = FindDominatingBoundsCheck(checks, dominated, blockIndex);
+    if (!dominating)
+        return false;
+
+    if (dominating == dominated) {
+        // We didn't find a dominating bounds check.
+        return true;
+    }
+
     // We found two bounds checks with the same hash number, but we still have
     // to make sure the lengths and index terms are equal.
     if (dominating->length() != dominated->length())
@@ -1177,6 +1196,99 @@ TryEliminateBoundsCheck(MBoundsCheck *dominating, MBoundsCheck *dominated, bool
     return true;
 }
 
+static void
+TryEliminateTypeBarrierFromTest(MTypeBarrier *barrier, bool filtersNull, bool filtersUndefined,
+                                MTest *test, BranchDirection direction, bool *eliminated)
+{
+    JS_ASSERT(filtersNull || filtersUndefined);
+
+    // Watch for code patterns similar to 'if (x.f) { ... = x.f }'.  If x.f
+    // is either an object or null/undefined, there will be a type barrier on
+    // the latter read as the null/undefined value is never realized there.
+    // The type barrier can be eliminated, however, by looking at tests
+    // performed on the result of the first operation that filter out all
+    // types that have been seen in the first access but not the second.
+
+    // A test 'if (x.f)' filters both null and undefined.
+    if (test->getOperand(0) == barrier->input() && direction == TRUE_BRANCH) {
+        *eliminated = true;
+        barrier->replaceAllUsesWith(barrier->input());
+        return;
+    }
+
+    if (!test->getOperand(0)->isCompare())
+        return;
+
+    MCompare *compare = test->getOperand(0)->toCompare();
+    MCompare::CompareType compareType = compare->compareType();
+
+    if (compareType != MCompare::Compare_Undefined && compareType != MCompare::Compare_Null)
+        return;
+    if (compare->getOperand(0) != barrier->input())
+        return;
+
+    JSOp op = compare->jsop();
+    JS_ASSERT(op == JSOP_EQ || op == JSOP_STRICTEQ ||
+              op == JSOP_NE || op == JSOP_STRICTNE);
+
+    if ((direction == TRUE_BRANCH) != (op == JSOP_NE || op == JSOP_STRICTNE))
+        return;
+
+    // A test 'if (x.f != null)' or 'if (x.f != undefined)' filters both null
+    // and undefined. If strict equality is used, only the specified rhs is
+    // tested for.
+    if (op == JSOP_STRICTEQ || op == JSOP_STRICTNE) {
+        if (compareType == MCompare::Compare_Undefined && !filtersUndefined)
+            return;
+        if (compareType == MCompare::Compare_Null && !filtersNull)
+            return;
+    }
+
+    *eliminated = true;
+    barrier->replaceAllUsesWith(barrier->input());
+}
+
+static bool
+TryEliminateTypeBarrier(MTypeBarrier *barrier, bool *eliminated)
+{
+    JS_ASSERT(!*eliminated);
+
+    const types::StackTypeSet *barrierTypes = barrier->typeSet();
+    const types::StackTypeSet *inputTypes = barrier->input()->typeSet();
+
+    if (!barrierTypes || !inputTypes)
+        return true;
+
+    bool filtersNull = barrierTypes->filtersType(inputTypes, types::Type::NullType());
+    bool filtersUndefined = barrierTypes->filtersType(inputTypes, types::Type::UndefinedType());
+
+    if (!filtersNull && !filtersUndefined)
+        return true;
+
+    MBasicBlock *block = barrier->block();
+    while (true) {
+        BranchDirection direction;
+        MTest *test = block->immediateDominatorBranch(&direction);
+
+        if (test) {
+            TryEliminateTypeBarrierFromTest(barrier, filtersNull, filtersUndefined,
+                                            test, direction, eliminated);
+        }
+
+        MBasicBlock *previous = block->immediateDominator();
+        if (previous == block)
+            break;
+        block = previous;
+    }
+
+    return true;
+}
+
+// Eliminate checks which are redundant given each other or other instructions.
+//
+// A type barrier is considered redundant if all missing types have been tested
+// for by earlier control instructions.
+//
 // A bounds check is considered redundant if it's dominated by another bounds
 // check with the same length and the indexes differ by only a constant amount.
 // In this case we eliminate the redundant bounds check and update the other one
@@ -1186,7 +1298,7 @@ TryEliminateBoundsCheck(MBoundsCheck *dominating, MBoundsCheck *dominated, bool
 // differences in constant offset, this offers a fast way to find redundant
 // checks.
 bool
-ion::EliminateRedundantBoundsChecks(MIRGraph &graph)
+ion::EliminateRedundantChecks(MIRGraph &graph)
 {
     BoundsCheckMap checks;
 
@@ -1220,41 +1332,18 @@ ion::EliminateRedundantBoundsChecks(MIRGraph &graph)
         }
 
         for (MDefinitionIterator iter(block); iter; ) {
-            if (!iter->isBoundsCheck()) {
-                iter++;
-                continue;
-            }
-
-            MBoundsCheck *check = iter->toBoundsCheck();
-
-            // Replace all uses of the bounds check with the actual index.
-            // This is (a) necessary, because we can coalesce two different
-            // bounds checks and would otherwise use the wrong index and
-            // (b) helps register allocation. Note that this is safe since
-            // no other pass after bounds check elimination moves instructions.
-            check->replaceAllUsesWith(check->index());
-
-            if (!check->isMovable()) {
-                iter++;
-                continue;
-            }
-
-            MBoundsCheck *dominating = FindDominatingBoundsCheck(checks, check, index);
-            if (!dominating)
-                return false;
-
-            if (dominating == check) {
-                // We didn't find a dominating bounds check.
-                iter++;
-                continue;
-            }
-
             bool eliminated = false;
-            if (!TryEliminateBoundsCheck(dominating, check, &eliminated))
+
+            if (iter->isBoundsCheck()) {
+                if (!TryEliminateBoundsCheck(checks, index, iter->toBoundsCheck(), &eliminated))
                     return false;
+            } else if (iter->isTypeBarrier()) {
+                if (!TryEliminateTypeBarrier(iter->toTypeBarrier(), &eliminated))
+                    return false;
+            }
 
             if (eliminated)
-                iter = check->block()->discardDefAt(iter);
+                iter = block->discardDefAt(iter);
             else
                 iter++;
         }

js/src/ion/IonAnalysis.h

@@ -55,7 +55,7 @@ void
 AssertExtendedGraphCoherency(MIRGraph &graph);
 
 bool
-EliminateRedundantBoundsChecks(MIRGraph &graph);
+EliminateRedundantChecks(MIRGraph &graph);
 
 class MDefinition;
 

js/src/ion/IonBuilder.cpp

@@ -5103,6 +5103,11 @@ IonBuilder::pushTypeBarrier(MInstruction *ins, types::StackTypeSet *actual,
             current->pop();
             current->add(replace);
             current->push(replace);
+            if (replace->acceptsTypeSet())
+                replace->setTypeSet(cloneTypeSet(actual));
+        } else {
+            if (ins->acceptsTypeSet())
+                ins->setTypeSet(cloneTypeSet(actual));
         }
         return true;
     }
@@ -5151,7 +5156,7 @@ IonBuilder::pushTypeBarrier(MInstruction *ins, types::StackTypeSet *actual,
 // Test the type of values returned by a VM call. This is an optimized version
 // of calling TypeScript::Monitor inside such stubs.
 void
-IonBuilder::monitorResult(MInstruction *ins, types::TypeSet *barrier, types::TypeSet *types)
+IonBuilder::monitorResult(MInstruction *ins, types::TypeSet *barrier, types::StackTypeSet *types)
 {
     // MonitorTypes is redundant if we will also add a type barrier.
     if (barrier)
@@ -7081,8 +7086,8 @@ IonBuilder::addShapeGuard(MDefinition *obj, const UnrootedShape shape, BailoutKi
     return guard;
 }
 
-const types::TypeSet *
-IonBuilder::cloneTypeSet(const types::TypeSet *types)
+const types::StackTypeSet *
+IonBuilder::cloneTypeSet(const types::StackTypeSet *types)
 {
     if (!js_IonOptions.parallelCompilation)
         return types;

js/src/ion/IonBuilder.h

@@ -288,7 +288,7 @@ class IonBuilder : public MIRGenerator
     bool initScopeChain();
     bool pushConstant(const Value &v);
     bool pushTypeBarrier(MInstruction *ins, types::StackTypeSet *actual, types::StackTypeSet *observed);
-    void monitorResult(MInstruction *ins, types::TypeSet *barrier, types::TypeSet *types);
+    void monitorResult(MInstruction *ins, types::TypeSet *barrier, types::StackTypeSet *types);
 
     JSObject *getSingletonPrototype(JSFunction *target);
 
@@ -470,7 +470,7 @@ class IonBuilder : public MIRGenerator
                            MBasicBlock *bottom,
                            Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns);
 
-    const types::TypeSet *cloneTypeSet(const types::TypeSet *types);
+    const types::StackTypeSet *cloneTypeSet(const types::StackTypeSet *types);
 
     // A builder is inextricably tied to a particular script.
     HeapPtrScript script_;

js/src/ion/MIR.cpp

@@ -367,7 +367,7 @@ MConstantElements::printOpcode(FILE *fp)
 }
 
 MParameter *
-MParameter::New(int32_t index, const types::TypeSet *types)
+MParameter::New(int32_t index, const types::StackTypeSet *types)
 {
     return new MParameter(index, types);
 }

js/src/ion/MIR.h

@@ -443,6 +443,15 @@ class MDefinition : public MNode
         resultType_ = type;
     }
 
+    virtual bool acceptsTypeSet() const {
+        return false;
+    }
+    virtual void setTypeSet(const types::StackTypeSet *types) {
+    }
+    virtual const types::StackTypeSet *typeSet() const {
+        return NULL;
+    }
+
     MDefinition *dependency() const {
         return dependency_;
     }
@@ -663,12 +672,12 @@ class MConstant : public MNullaryInstruction
 class MParameter : public MNullaryInstruction
 {
     int32_t index_;
-    const types::TypeSet *typeSet_;
+    const types::StackTypeSet *typeSet_;
 
   public:
     static const int32_t THIS_SLOT = -1;
 
-    MParameter(int32_t index, const types::TypeSet *types)
+    MParameter(int32_t index, const types::StackTypeSet *types)
       : index_(index),
         typeSet_(types)
     {
@@ -677,12 +686,12 @@ class MParameter : public MNullaryInstruction
 
   public:
     INSTRUCTION_HEADER(Parameter)
-    static MParameter *New(int32_t index, const types::TypeSet *types);
+    static MParameter *New(int32_t index, const types::StackTypeSet *types);
 
     int32_t index() const {
         return index_;
     }
-    const types::TypeSet *typeSet() const {
+    const types::StackTypeSet *typeSet() const {
         return typeSet_;
     }
     void printOpcode(FILE *fp);
@@ -4117,10 +4126,11 @@ class MLoadFixedSlot
     public SingleObjectPolicy
 {
     size_t slot_;
+    const types::StackTypeSet *types_;
 
   protected:
     MLoadFixedSlot(MDefinition *obj, size_t slot)
-      : MUnaryInstruction(obj), slot_(slot)
+      : MUnaryInstruction(obj), slot_(slot), types_(NULL)
     {
         setResultType(MIRType_Value);
         setMovable();
@@ -4137,6 +4147,16 @@ class MLoadFixedSlot
         return this;
     }
 
+    virtual bool acceptsTypeSet() const {
+        return true;
+    }
+    virtual void setTypeSet(const types::StackTypeSet *types) {
+        types_ = types;
+    }
+    virtual const types::StackTypeSet *typeSet() const {
+        return types_;
+    }
+
     MDefinition *object() const {
         return getOperand(0);
     }
@@ -4697,10 +4717,12 @@ class MLoadSlot
     public SingleObjectPolicy
 {
     uint32_t slot_;
+    const types::StackTypeSet *types_;
 
     MLoadSlot(MDefinition *slots, uint32_t slot)
       : MUnaryInstruction(slots),
-        slot_(slot)
+        slot_(slot),
+        types_(NULL)
     {
         setResultType(MIRType_Value);
         setMovable();
@@ -4723,6 +4745,17 @@ class MLoadSlot
     uint32_t slot() const {
         return slot_;
     }
+
+    virtual bool acceptsTypeSet() const {
+        return true;
+    }
+    virtual void setTypeSet(const types::StackTypeSet *types) {
+        types_ = types;
+    }
+    virtual const types::StackTypeSet *typeSet() const {
+        return types_;
+    }
+
     bool congruentTo(MDefinition * const &ins) const {
         if (!ins->isLoadSlot())
             return false;
@@ -5564,9 +5597,9 @@ class MGetArgument
 class MTypeBarrier : public MUnaryInstruction
 {
     BailoutKind bailoutKind_;
-    const types::TypeSet *typeSet_;
+    const types::StackTypeSet *typeSet_;
 
-    MTypeBarrier(MDefinition *def, const types::TypeSet *types)
+    MTypeBarrier(MDefinition *def, const types::StackTypeSet *types)
       : MUnaryInstruction(def),
         typeSet_(types)
     {
@@ -5581,7 +5614,7 @@ class MTypeBarrier : public MUnaryInstruction
   public:
     INSTRUCTION_HEADER(TypeBarrier)
 
-    static MTypeBarrier *New(MDefinition *def, const types::TypeSet *types) {
+    static MTypeBarrier *New(MDefinition *def, const types::StackTypeSet *types) {
         return new MTypeBarrier(def, types);
     }
     bool congruentTo(MDefinition * const &def) const {
@@ -5593,7 +5626,7 @@ class MTypeBarrier : public MUnaryInstruction
     BailoutKind bailoutKind() const {
         return bailoutKind_;
     }
-    const types::TypeSet *typeSet() const {
+    const types::StackTypeSet *typeSet() const {
         return typeSet_;
     }
     AliasSet getAliasSet() const {
@@ -5610,9 +5643,9 @@ class MTypeBarrier : public MUnaryInstruction
 // TypeScript::Monitor inside these stubs.
 class MMonitorTypes : public MUnaryInstruction
 {
-    const types::TypeSet *typeSet_;
+    const types::StackTypeSet *typeSet_;
 
-    MMonitorTypes(MDefinition *def, const types::TypeSet *types)
+    MMonitorTypes(MDefinition *def, const types::StackTypeSet *types)
       : MUnaryInstruction(def),
         typeSet_(types)
     {
@@ -5624,13 +5657,13 @@ class MMonitorTypes : public MUnaryInstruction
   public:
     INSTRUCTION_HEADER(MonitorTypes)
 
-    static MMonitorTypes *New(MDefinition *def, const types::TypeSet *types) {
+    static MMonitorTypes *New(MDefinition *def, const types::StackTypeSet *types) {
         return new MMonitorTypes(def, types);
     }
     MDefinition *input() const {
         return getOperand(0);
     }
-    const types::TypeSet *typeSet() const {
+    const types::StackTypeSet *typeSet() const {
         return typeSet_;
     }
     AliasSet getAliasSet() const {

js/src/jit-test/tests/ion/eliminate-type-barrier.js

@@ -0,0 +1,41 @@
+function foo(a) {
+  var y = 0;
+  for (var i = 0; i < 10; i++) {
+    var x = a[i];
+    z = x.f;
+    if (x.h != null)
+      y = x.f.g;
+  }
+  return y;
+}
+
+function foo2(a) {
+  var y = 0;
+  for (var i = 0; i < 10; i++) {
+    var x = a[i];
+    if (x.f !== undefined) {
+      if (x.h != null)
+        y = x.f.g;
+    }
+  }
+  return y;
+}
+
+a = [];
+for (var i = 0; i < 10; i++)
+  a[i] = {f:null, h:null};
+for (var i = 0; i < 10; i++) {
+  a[i].f = {g:0};
+  a[i].h = {};
+}
+var q = a[0].h;
+a[0].f = null;
+a[0].h = null;
+
+foo(a);
+foo2(a);
+
+a[0].h = q;
+
+try { foo(a); } catch (e) {}
+try { foo2(a); } catch (e) {}

js/src/jsinfer.cpp

@@ -458,13 +458,13 @@ StackTypeSet::make(JSContext *cx, const char *name)
     return res;
 }
 
-const TypeSet *
+const StackTypeSet *
 TypeSet::clone(LifoAlloc *alloc) const
 {
     unsigned objectCount = baseObjectCount();
     unsigned capacity = (objectCount >= 2) ? HashSetCapacity(objectCount) : 0;
 
-    TypeSet *res = alloc->new_<TypeSet>();
+    StackTypeSet *res = alloc->new_<StackTypeSet>();
     if (!res)
         return NULL;
 
@@ -1865,6 +1865,33 @@ StackTypeSet::knownNonStringPrimitive()
     return true;
 }
 
+bool
+StackTypeSet::filtersType(const StackTypeSet *other, Type filteredType) const
+{
+    if (other->unknown())
+        return unknown();
+
+    for (TypeFlags flag = 1; flag < TYPE_FLAG_ANYOBJECT; flag <<= 1) {
+        Type type = Type::PrimitiveType(TypeFlagPrimitive(flag));
+        if (type != filteredType && other->hasType(type) && !hasType(type))
+            return false;
+    }
+
+    if (other->unknownObject())
+        return unknownObject();
+
+    for (size_t i = 0; i < other->getObjectCount(); i++) {
+        TypeObjectKey *key = other->getObject(i);
+        if (key) {
+            Type type = Type::ObjectType(key);
+            if (type != filteredType && !hasType(type))
+                return false;
+        }
+    }
+
+    return true;
+}
+
 bool
 HeapTypeSet::knownSubset(JSContext *cx, TypeSet *other)
 {

js/src/jsinfer.h

@@ -470,7 +470,7 @@ class TypeSet
      * Clone a type set into an arbitrary allocator. The result should not be
      * modified further.
      */
-    const TypeSet *clone(LifoAlloc *alloc) const;
+    const StackTypeSet *clone(LifoAlloc *alloc) const;
 
     /*
      * Add a type to this set, calling any constraint handlers if this is a new
@@ -604,6 +604,12 @@ class StackTypeSet : public TypeSet
     /* Whether any objects in the type set needs a barrier on id. */
     bool propertyNeedsBarrier(JSContext *cx, jsid id);
 
+    /*
+     * Whether this set contains all types in other, except (possibly) the
+     * specified type.
+     */
+    bool filtersType(const StackTypeSet *other, Type type) const;
+
     /*
      * Get whether this type only contains non-string primitives:
      * null/undefined/int/double, or some combination of those.