Bug 1316850 - clean up void handling. r=bbouvier
authorLars T Hansen <lhansen@mozilla.com>
Wed, 16 Nov 2016 11:29:46 +0100
changeset 439864 0da56750be97361ba349e591b699b66911a0ce9c
parent 439863 a8873fe7b7c83793a1015555a7b641e0577094c2
child 439865 df748be239499d9a8f88df38988d89acab774f7a
push id36113
push userbmo:afarre@mozilla.com
push dateWed, 16 Nov 2016 18:15:04 +0000
reviewersbbouvier
bugs1316850
milestone53.0a1
Bug 1316850 - clean up void handling. r=bbouvier
js/src/wasm/WasmBaselineCompile.cpp
--- a/js/src/wasm/WasmBaselineCompile.cpp
+++ b/js/src/wasm/WasmBaselineCompile.cpp
@@ -1123,19 +1123,18 @@ class BaseCompiler
             break;
           case Stk::LocalI32:
             loadLocalI32(r, src);
             break;
           case Stk::RegisterI32:
             loadRegisterI32(r, src);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: Expected int on stack");
+            MOZ_CRASH("Compiler bug: Expected I32 on stack");
         }
     }
 
     // TODO / OPTIMIZE: Refactor loadI64, loadF64, and loadF32 in the
     // same way as loadI32 to avoid redundant dispatch in callers of
     // these load() functions.  (Bug 1316816, also see annotations on
     // popI64 et al below.)
 
@@ -1150,19 +1149,18 @@ class BaseCompiler
           case Stk::LocalI64:
             loadFromFrameI64(r, frameOffsetFromSlot(src.slot(), MIRType::Int64));
             break;
           case Stk::RegisterI64:
             if (src.i64reg().reg != r)
                 masm.move64(src.i64reg().reg, r);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: Expected int on stack");
+            MOZ_CRASH("Compiler bug: Expected I64 on stack");
         }
     }
 
 #ifdef JS_NUNBOX32
     void loadI64Low(Register r, Stk& src) {
         switch (src.kind()) {
           case Stk::ConstI64:
             masm.move32(Imm64(src.i64val()).low(), r);
@@ -1173,19 +1171,18 @@ class BaseCompiler
           case Stk::LocalI64:
             loadFromFrameI32(r, frameOffsetFromSlot(src.slot(), MIRType::Int64) - INT64LOW_OFFSET);
             break;
           case Stk::RegisterI64:
             if (src.i64reg().reg.low != r)
                 masm.move32(src.i64reg().reg.low, r);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: Expected int on stack");
+            MOZ_CRASH("Compiler bug: Expected I64 on stack");
         }
     }
 
     void loadI64High(Register r, Stk& src) {
         switch (src.kind()) {
           case Stk::ConstI64:
             masm.move32(Imm64(src.i64val()).hi(), r);
             break;
@@ -1195,19 +1192,18 @@ class BaseCompiler
           case Stk::LocalI64:
             loadFromFrameI32(r, frameOffsetFromSlot(src.slot(), MIRType::Int64) - INT64HIGH_OFFSET);
             break;
           case Stk::RegisterI64:
             if (src.i64reg().reg.high != r)
                 masm.move32(src.i64reg().reg.high, r);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: Expected int on stack");
+            MOZ_CRASH("Compiler bug: Expected I64 on stack");
         }
     }
 #endif
 
     void loadF64(FloatRegister r, Stk& src) {
         switch (src.kind()) {
           case Stk::ConstF64:
             masm.loadConstantDouble(src.f64val(), r);
@@ -1218,19 +1214,18 @@ class BaseCompiler
           case Stk::LocalF64:
             loadFromFrameF64(r, frameOffsetFromSlot(src.slot(), MIRType::Double));
             break;
           case Stk::RegisterF64:
             if (src.f64reg().reg != r)
                 masm.moveDouble(src.f64reg().reg, r);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: expected double on stack");
+            MOZ_CRASH("Compiler bug: expected F64 on stack");
         }
     }
 
     void loadF32(FloatRegister r, Stk& src) {
         switch (src.kind()) {
           case Stk::ConstF32:
             masm.loadConstantFloat32(src.f32val(), r);
             break;
@@ -1240,19 +1235,18 @@ class BaseCompiler
           case Stk::LocalF32:
             loadFromFrameF32(r, frameOffsetFromSlot(src.slot(), MIRType::Float32));
             break;
           case Stk::RegisterF32:
             if (src.f32reg().reg != r)
                 masm.moveFloat32(src.f32reg().reg, r);
             break;
           case Stk::None:
-            break;
           default:
-            MOZ_CRASH("Compiler bug: expected float on stack");
+            MOZ_CRASH("Compiler bug: expected F32 on stack");
         }
     }
 
     // Flush all local and register value stack elements to memory.
     //
     // TODO / OPTIMIZE: As this is fairly expensive and causes worse
     // code to be emitted subsequently, it is useful to avoid calling
     // it.  (Bug 1316802)
@@ -1472,26 +1466,16 @@ class BaseCompiler
             break;
           case Stk::MemI32:
             masm.Pop(r.reg);
             break;
           case Stk::RegisterI32:
             moveI32(v.i32reg(), r);
             break;
           case Stk::None:
-            // This case crops up in situations where there's unreachable code that
-            // the type system interprets as "generating" a value of the correct type:
-            //
-            //   (if (return) E1 E2)                    type is type(E1) meet type(E2)
-            //   (if E (unreachable) (i32.const 1))     type is int
-            //   (if E (i32.const 1) (unreachable))     type is int
-            //
-            // It becomes silly to handle this throughout the code, so just handle it
-            // here even if that means weaker run-time checking.
-            break;
           default:
             MOZ_CRASH("Compiler bug: expected int on stack");
         }
     }
 
     MOZ_MUST_USE RegI32 popI32() {
         Stk& v = stk_.back();
         RegI32 r;
@@ -1534,18 +1518,16 @@ class BaseCompiler
             masm.Pop(r.reg.low);
             masm.Pop(r.reg.high);
 #endif
             break;
           case Stk::RegisterI64:
             moveI64(v.i64reg(), r);
             break;
           case Stk::None:
-            // See popI32()
-            break;
           default:
             MOZ_CRASH("Compiler bug: expected long on stack");
         }
     }
 
     MOZ_MUST_USE RegI64 popI64() {
         Stk& v = stk_.back();
         RegI64 r;
@@ -1588,18 +1570,16 @@ class BaseCompiler
             break;
           case Stk::MemF64:
             masm.Pop(r.reg);
             break;
           case Stk::RegisterF64:
             moveF64(v.f64reg(), r);
             break;
           case Stk::None:
-            // See popI32()
-            break;
           default:
             MOZ_CRASH("Compiler bug: expected double on stack");
         }
     }
 
     MOZ_MUST_USE RegF64 popF64() {
         Stk& v = stk_.back();
         RegF64 r;
@@ -1637,18 +1617,16 @@ class BaseCompiler
             break;
           case Stk::MemF32:
             masm.Pop(r.reg);
             break;
           case Stk::RegisterF32:
             moveF32(v.f32reg(), r);
             break;
           case Stk::None:
-            // See popI32()
-            break;
           default:
             MOZ_CRASH("Compiler bug: expected float on stack");
         }
     }
 
     MOZ_MUST_USE RegF32 popF32() {
         Stk& v = stk_.back();
         RegF32 r;
@@ -1697,91 +1675,100 @@ class BaseCompiler
     // On the other hand, we sync() before every block and only the
     // JoinReg is live out of the block.  But on the way out, we
     // currently pop the JoinReg before freeing regs to be discarded,
     // so there is a real risk of some pointless shuffling there.  If
     // we instead integrate the popping of the join reg into the
     // popping of the stack we can just use the JoinReg as it will
     // become available in that process.
 
-    MOZ_MUST_USE AnyReg popJoinReg() {
-        switch (stk_.back().kind()) {
-          case Stk::RegisterI32:
-          case Stk::ConstI32:
-          case Stk::MemI32:
-          case Stk::LocalI32:
+    MOZ_MUST_USE AnyReg popJoinRegUnlessVoid(ExprType type) {
+        switch (type) {
+          case ExprType::Void: {
+            return AnyReg();
+          }
+          case ExprType::I32: {
+            DebugOnly<Stk::Kind> k(stk_.back().kind());
+            MOZ_ASSERT(k == Stk::RegisterI32 || k == Stk::ConstI32 || k == Stk::MemI32 ||
+                       k == Stk::LocalI32);
             return AnyReg(popI32(joinRegI32));
-          case Stk::RegisterI64:
-          case Stk::ConstI64:
-          case Stk::MemI64:
-          case Stk::LocalI64:
+          }
+          case ExprType::I64: {
+            DebugOnly<Stk::Kind> k(stk_.back().kind());
+            MOZ_ASSERT(k == Stk::RegisterI64 || k == Stk::ConstI64 || k == Stk::MemI64 ||
+                       k == Stk::LocalI64);
             return AnyReg(popI64(joinRegI64));
-          case Stk::RegisterF64:
-          case Stk::ConstF64:
-          case Stk::MemF64:
-          case Stk::LocalF64:
+          }
+          case ExprType::F64: {
+            DebugOnly<Stk::Kind> k(stk_.back().kind());
+            MOZ_ASSERT(k == Stk::RegisterF64 || k == Stk::ConstF64 || k == Stk::MemF64 ||
+                       k == Stk::LocalF64);
             return AnyReg(popF64(joinRegF64));
-          case Stk::RegisterF32:
-          case Stk::ConstF32:
-          case Stk::MemF32:
-          case Stk::LocalF32:
+          }
+          case ExprType::F32: {
+            DebugOnly<Stk::Kind> k(stk_.back().kind());
+            MOZ_ASSERT(k == Stk::RegisterF32 || k == Stk::ConstF32 || k == Stk::MemF32 ||
+                       k == Stk::LocalF32);
             return AnyReg(popF32(joinRegF32));
-          case Stk::None:
-            stk_.popBack();
-            return AnyReg();
-          default:
-            MOZ_CRASH("Compiler bug: unexpected value on stack");
+          }
+          default: {
+            MOZ_CRASH("Compiler bug: unexpected expression type");
+          }
         }
     }
 
-    MOZ_MUST_USE AnyReg allocJoinReg(ExprType type) {
+    // If we ever start not sync-ing on entry to Block (but instead try to sync
+    // lazily) then this may start asserting because it does not spill the
+    // joinreg if the joinreg is already allocated.  Note, it *can't* spill the
+    // joinreg in the contexts it's being used, so some other solution will need
+    // to be found.
+
+    MOZ_MUST_USE AnyReg captureJoinRegUnlessVoid(ExprType type) {
         switch (type) {
           case ExprType::I32:
             allocGPR(joinRegI32.reg);
             return AnyReg(joinRegI32);
           case ExprType::I64:
             allocInt64(joinRegI64.reg);
             return AnyReg(joinRegI64);
           case ExprType::F32:
             allocFPU(joinRegF32.reg);
             return AnyReg(joinRegF32);
           case ExprType::F64:
             allocFPU(joinRegF64.reg);
             return AnyReg(joinRegF64);
           case ExprType::Void:
-            MOZ_CRASH("Compiler bug: allocating void join reg");
+            return AnyReg();
           default:
             MOZ_CRASH("Compiler bug: unexpected type");
         }
     }
 
-    void pushJoinReg(AnyReg r) {
+    void pushJoinRegUnlessVoid(AnyReg r) {
         switch (r.tag) {
           case AnyReg::NONE:
-            MOZ_CRASH("Compile bug: attempting to push void");
             break;
           case AnyReg::I32:
             pushI32(r.i32());
             break;
           case AnyReg::I64:
             pushI64(r.i64());
             break;
           case AnyReg::F64:
             pushF64(r.f64());
             break;
           case AnyReg::F32:
             pushF32(r.f32());
             break;
         }
     }
 
-    void freeJoinReg(AnyReg r) {
+    void freeJoinRegUnlessVoid(AnyReg r) {
         switch (r.tag) {
           case AnyReg::NONE:
-            MOZ_CRASH("Compile bug: attempting to free void reg");
             break;
           case AnyReg::I32:
             freeI32(r.i32());
             break;
           case AnyReg::I64:
             freeI64(r.i64());
             break;
           case AnyReg::F64:
@@ -4843,36 +4830,37 @@ BaseCompiler::emitBlock()
 
 void
 BaseCompiler::endBlock(ExprType type, bool isFunctionBody)
 {
     Control& block = controlItem(0);
 
     // Save the value.
     AnyReg r;
-    if (!deadCode_ && !IsVoid(type))
-        r = popJoinReg();
+    if (!deadCode_)
+        r = popJoinRegUnlessVoid(type);
 
     // Leave the block.
     popStackOnBlockExit(block.framePushed);
 
     // Bind after cleanup: branches out will have popped the stack.
     if (block.label->used()) {
         masm.bind(block.label);
-        if (deadCode_ && !IsVoid(type))
-            r = allocJoinReg(type);
+        // No value was provided by the fallthrough but the branch out will
+        // have stored one in joinReg, so capture that.
+        if (deadCode_)
+            r = captureJoinRegUnlessVoid(type);
         deadCode_ = false;
     }
 
     MOZ_ASSERT(stk_.length() == block.stackSize);
 
-    // Retain the value stored in joinReg by all paths.
+    // Retain the value stored in joinReg by all paths, if there are any.
     if (!deadCode_) {
-        if (!IsVoid(type))
-            pushJoinReg(r);
+        pushJoinRegUnlessVoid(r);
 
         if (isFunctionBody)
             doReturn(func_.sig().ret());
     }
 
     popControl();
 }
 
@@ -4901,28 +4889,28 @@ BaseCompiler::emitLoop()
 }
 
 void
 BaseCompiler::endLoop(ExprType type)
 {
     Control& block = controlItem(0);
 
     AnyReg r;
-    if (!deadCode_ && !IsVoid(type))
-        r = popJoinReg();
+    if (!deadCode_)
+        r = popJoinRegUnlessVoid(type);
 
     popStackOnBlockExit(block.framePushed);
 
     MOZ_ASSERT(stk_.length() == block.stackSize);
 
     popControl();
 
     // Retain the value stored in joinReg by all paths.
-    if (!deadCode_ && !IsVoid(type))
-        pushJoinReg(r);
+    if (!deadCode_)
+        pushJoinRegUnlessVoid(r);
 }
 
 // The bodies of the "then" and "else" arms can be arbitrary sequences
 // of expressions, they push control and increment the nesting and can
 // even be targeted by jumps.  A branch to the "if" block branches to
 // the exit of the if, ie, it's like "break".  Consider:
 //
 //      (func (result i32)
@@ -4998,33 +4986,33 @@ BaseCompiler::emitElse()
 
     // See comment in endIfThenElse, below.
 
     // Exit the "then" branch.
 
     ifThenElse.deadThenBranch = deadCode_;
 
     AnyReg r;
-    if (!deadCode_ && !IsVoid(thenType))
-        r = popJoinReg();
+    if (!deadCode_)
+        r = popJoinRegUnlessVoid(thenType);
 
     popStackOnBlockExit(ifThenElse.framePushed);
 
     if (!deadCode_)
         masm.jump(ifThenElse.label);
 
     if (ifThenElse.otherLabel->used())
         masm.bind(ifThenElse.otherLabel);
 
     // Reset to the "else" branch.
 
     MOZ_ASSERT(stk_.length() == ifThenElse.stackSize);
 
-    if (!deadCode_ && !IsVoid(thenType))
-        freeJoinReg(r);
+    if (!deadCode_)
+        freeJoinRegUnlessVoid(r);
 
     deadCode_ = ifThenElse.deadOnArrival;
 
     return true;
 }
 
 void
 BaseCompiler::endIfThenElse(ExprType type)
@@ -5033,37 +5021,42 @@ BaseCompiler::endIfThenElse(ExprType typ
 
     // The expression type is not a reliable guide to what we'll find
     // on the stack, we could have (if E (i32.const 1) (unreachable))
     // in which case the "else" arm is AnyType but the type of the
     // full expression is I32.  So restore whatever's there, not what
     // we want to find there.  The "then" arm has the same constraint.
 
     AnyReg r;
-    if (!deadCode_ && !IsVoid(type))
-        r = popJoinReg();
+
+    if (!deadCode_)
+        r = popJoinRegUnlessVoid(type);
 
     popStackOnBlockExit(ifThenElse.framePushed);
 
     if (ifThenElse.label->used())
         masm.bind(ifThenElse.label);
 
-    if (!ifThenElse.deadOnArrival &&
-        (!ifThenElse.deadThenBranch || !deadCode_ || ifThenElse.label->bound())) {
-        if (deadCode_ && !IsVoid(type))
-            r = allocJoinReg(type);
+    bool joinLive = !ifThenElse.deadOnArrival &&
+                    (!ifThenElse.deadThenBranch || !deadCode_ || ifThenElse.label->bound());
+
+    if (joinLive) {
+        // No value was provided by the "then" path but capture the one
+        // provided by the "else" path.
+        if (deadCode_)
+            r = captureJoinRegUnlessVoid(type);
         deadCode_ = false;
     }
 
     MOZ_ASSERT(stk_.length() == ifThenElse.stackSize);
 
     popControl();
 
-    if (!deadCode_ && !IsVoid(type))
-        pushJoinReg(r);
+    if (!deadCode_)
+        pushJoinRegUnlessVoid(r);
 }
 
 bool
 BaseCompiler::emitEnd()
 {
     LabelKind kind;
     ExprType type;
     Nothing unused_value;
@@ -5093,28 +5086,25 @@ BaseCompiler::emitBr()
     if (deadCode_)
         return true;
 
     Control& target = controlItem(relativeDepth);
 
     // Save any value in the designated join register, where the
     // normal block exit code will also leave it.
 
-    AnyReg r;
-    if (!IsVoid(type))
-        r = popJoinReg();
+    AnyReg r = popJoinRegUnlessVoid(type);
 
     popStackBeforeBranch(target.framePushed);
     masm.jump(target.label);
 
     // The register holding the join value is free for the remainder
     // of this block.
 
-    if (!IsVoid(type))
-        freeJoinReg(r);
+    freeJoinRegUnlessVoid(r);
 
     deadCode_ = true;
 
     popValueStackTo(ctl_.back().stackSize);
 
     return true;
 }
 
@@ -5141,32 +5131,29 @@ BaseCompiler::emitBrIf()
 
     // Condition value is on top, always I32.
     RegI32 rc = popI32();
 
     maybeUnreserveJoinRegI(type);
 
     // Save any value in the designated join register, where the
     // normal block exit code will also leave it.
-    AnyReg r;
-    if (!IsVoid(type))
-        r = popJoinReg();
+    AnyReg r = popJoinRegUnlessVoid(type);
 
     Label notTaken;
     masm.branch32(Assembler::Equal, rc.reg, Imm32(0), &notTaken);
     popStackBeforeBranch(target.framePushed);
     masm.jump(target.label);
     masm.bind(&notTaken);
 
     // This register is free in the remainder of the block.
     freeI32(rc);
 
     // br_if returns its value(s).
-    if (!IsVoid(type))
-        pushJoinReg(r);
+    pushJoinRegUnlessVoid(r);
 
     return true;
 }
 
 bool
 BaseCompiler::emitBrTable()
 {
     uint32_t tableLength;
@@ -5200,19 +5187,17 @@ BaseCompiler::emitBrTable()
     // Don't use joinReg for rc
     maybeReserveJoinRegI(type);
 
     // Table switch value always on top.
     RegI32 rc = popI32();
 
     maybeUnreserveJoinRegI(type);
 
-    AnyReg r;
-    if (!IsVoid(type))
-        r = popJoinReg();
+    AnyReg r = popJoinRegUnlessVoid(type);
 
     Label dispatchCode;
     masm.branch32(Assembler::Below, rc.reg, Imm32(tableLength), &dispatchCode);
 
     // This is the out-of-range stub.  rc is dead here but we don't need it.
 
     popStackBeforeBranch(controlItem(defaultDepth).framePushed);
     masm.jump(controlItem(defaultDepth).label);
@@ -5248,18 +5233,17 @@ BaseCompiler::emitBrTable()
     masm.bind(&dispatchCode);
     tableSwitch(&theTable, rc);
 
     deadCode_ = true;
 
     // Clean up.
 
     freeI32(rc);
-    if (!IsVoid(type))
-        freeJoinReg(r);
+    freeJoinRegUnlessVoid(r);
 
     for (uint32_t i = 0; i < tableLength; i++)
         freeLabel(stubs[i]);
 
     popValueStackTo(ctl_.back().stackSize);
 
     return true;
 }