Subzero, Wasm: Link and run torture tests; bug fixes.

src/WasmTranslator.cpp

Index: src/WasmTranslator.cpp
diff --git a/src/WasmTranslator.cpp b/src/WasmTranslator.cpp
index 6c1ed24659c1307cf613a8fd2233c9da218f6167..156dcee50b07e4fe12f18562b22bade9c694e6de 100644
--- a/src/WasmTranslator.cpp
+++ b/src/WasmTranslator.cpp
@@ -58,6 +58,8 @@ using v8::internal::wasm::DecodeWasmModule;
 #define LOG(Expr) log([&](Ostream & out) { Expr; })
 
 namespace {
+// 64KB
+const uint32_t WASM_PAGE_SIZE = 64 << 10;
 
 std::string toStdString(WasmName Name) {
   return std::string(Name.name, Name.length);
@@ -312,7 +314,7 @@ public:
     LOG(out << ") = ");
 
     const auto &InEdges = Control.toCfgNode()->getInEdges();
-    assert(Count == InEdges.size());
+    //assert(Count == InEdges.size());

[John, 2016/04/18 19:15:54]

why?

[Eric Holk, 2016/04/18 20:57:24]

This is leftover from one of my ill-fated debugging adventures. I've re-enabled
the assert.

The thinking was that Phi nodes are sometimes built incrementally by the WASM
decoder, so it may not always know all of the incoming edges to start with.

 
     assert(Count > 0);
 
@@ -329,7 +331,8 @@ public:
       Phi->addArgument(Op, InEdges[i]);
     }
     setDefiningInst(Dest, Phi);
-    Control.toCfgNode()->appendInst(Phi);
+    constexpr bool AllowPhisAnywhere = true;

[John, 2016/04/18 19:15:54]

Why is this needed? It seems weird to me to have a phi instruction in the middle
of a basic block...

[Eric Holk, 2016/04/18 20:57:25]

Another relic of a misguided debugging adventure. I went ahead and removed this
parameter, since I haven't actually found it useful.

The thinking was that the decoder may not know all the incoming edges to a node
ahead of time, so later on in the process it might have to add additional phis
to a block. Because the CfgNode class separates Phis from regular instructions,
they would still end up at the beginning. This flag simply let you add more Phis
after you'd already started adding normal instructions.

+    Control.toCfgNode()->appendInst(Phi, AllowPhisAnywhere);
     LOG(out << Node(Dest) << "\n");
     return OperandNode(Dest);
   }
@@ -466,7 +469,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ne, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32Eq:
@@ -475,7 +478,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Eq, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LtS:
@@ -484,7 +487,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Slt, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LeS:
@@ -493,7 +496,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sle, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GeU:
@@ -502,7 +505,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Uge, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LeU:
@@ -511,7 +514,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ule, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LtU:
@@ -520,7 +523,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ult, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GeS:
@@ -529,7 +532,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sge, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
     }
     case kExprI32GtS:
     case kExprI64GtS: {
@@ -537,7 +540,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sgt, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GtU:
@@ -546,7 +549,7 @@ public:
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ugt, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Ne:
@@ -555,7 +558,7 @@ public:
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Une, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Le:
@@ -564,7 +567,7 @@ public:
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Ule, TmpDest, Left, Right));
       Control()->appendInst(
-          InstCast::create(Func, InstCast::Sext, Dest, TmpDest));
+          InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     default:
@@ -585,7 +588,15 @@ public:
       auto *Tmp = makeVariable(IceType_i1);
       Control()->appendInst(InstIcmp::create(Func, InstIcmp::Eq, Tmp, Input,
                                              Ctx->getConstantInt32(0)));
-      Control()->appendInst(InstCast::create(Func, InstCast::Sext, Dest, Tmp));
+      Control()->appendInst(InstCast::create(Func, InstCast::Zext, Dest, Tmp));
+      break;
+    }
+    case kExprI64Eqz: {
+      Dest = makeVariable(IceType_i32);

[John, 2016/04/18 19:15:54]

i1?

[Eric Holk, 2016/04/18 20:57:24]

Eqz corresponds to the C expression `x == 0`. It's pretty common for code to go
on and treat this as a 32-bit integer and expect it to be either 0 or 1. This is
why all the boolean operations in this file compare into a temporary i1 but then
zero-extend it to an i32. Branches then truncate before doing the branch.

This is on my list of things to improve, because a lot of times we end up with
zext immediately followed by a truncate.

[John, 2016/04/18 21:10:01]

That (zext followed by truncates) also plague pnacl bitcodes. :)

+      auto *Tmp = makeVariable(IceType_i1);
+      Control()->appendInst(InstIcmp::create(Func, InstIcmp::Eq, Tmp, Input,
+                                             Ctx->getConstantInt64(0)));
+      Control()->appendInst(InstCast::create(Func, InstCast::Zext, Dest, Tmp));
       break;
     }
     case kExprF32Neg: {
@@ -673,7 +684,8 @@ public:
             << "\n");
 
     auto *CondBool = makeVariable(IceType_i1);
-    Ctrl->appendInst(InstCast::create(Func, InstCast::Trunc, CondBool, Cond));
+    Ctrl->appendInst(InstIcmp::create(Func, InstIcmp::Ne, CondBool, Cond,
+                                      Ctx->getConstantInt32(0)));
 
     Ctrl->appendInst(InstBr::create(Func, CondBool, *TrueNode, *FalseNode));
     return OperandNode(nullptr);
@@ -898,30 +910,45 @@ public:
   }
 
   Operand *sanitizeAddress(Operand *Base, uint32_t Offset) {
+    SizeT MemSize = Module->module->min_mem_pages * WASM_PAGE_SIZE;
+    SizeT MemMask = MemSize - 1;
+
+    bool ConstZeroBase = false;
+
     // first, add the index and the offset together.
-    if (0 != Offset) {
+    if (auto *ConstBase = llvm::dyn_cast<ConstantInteger32>(Base)) {
+      uint32_t RealOffset = Offset + ConstBase->getValue();
+      RealOffset &= MemMask;
+      Base = Ctx->getConstantInt32(RealOffset);
+      ConstZeroBase = (0 == RealOffset);
+    } else if (0 != Offset) {
       auto *Addr = makeVariable(IceType_i32);
       auto *OffsetConstant = Ctx->getConstantInt32(Offset);
       Control()->appendInst(InstArithmetic::create(Func, InstArithmetic::Add,
                                                    Addr, Base, OffsetConstant));
+
       Base = Addr;
     }
 
-    SizeT MemSize = Module->module->min_mem_pages * (16 << 10);
-    auto *WrappedAddr = makeVariable(IceType_i32);
-    Control()->appendInst(
-        InstArithmetic::create(Func, InstArithmetic::Add, WrappedAddr, Base,
-                               Ctx->getConstantInt32(MemSize)));
-
-    auto ClampedAddr = makeVariable(IceType_i32);
-    Control()->appendInst(
-        InstArithmetic::create(Func, InstArithmetic::And, ClampedAddr, Base,
-                               Ctx->getConstantInt32(MemSize - 1)));
+    if (!llvm::dyn_cast<ConstantInteger32>(Base)) {
+      auto ClampedAddr = makeVariable(IceType_i32);

[John, 2016/04/18 19:15:54]

auto *

Also, maybe you should use

Ice::getPointerType() instead?

[Eric Holk, 2016/04/18 20:57:24]

Done.

I switch to Ice::getPointerType here and in the rest of the function too.

+      Control()->appendInst(
+          InstArithmetic::create(Func, InstArithmetic::And, ClampedAddr, Base,
+                                 Ctx->getConstantInt32(MemSize - 1)));
+      Base = ClampedAddr;
+    }
 
-    auto RealAddr = Func->makeVariable(IceType_i32);
+    Ice::Operand *RealAddr = nullptr;
     auto MemBase = Ctx->getConstantSym(0, Ctx->getGlobalString("WASM_MEMORY"));
-    Control()->appendInst(InstArithmetic::create(
-        Func, InstArithmetic::Add, RealAddr, ClampedAddr, MemBase));
+    if (!ConstZeroBase) {
+      auto RealAddrV = Func->makeVariable(IceType_i32);
+      Control()->appendInst(InstArithmetic::create(
+          Func, InstArithmetic::Add, RealAddrV, Base, MemBase));
+
+      RealAddr = RealAddrV;
+    } else {
+      RealAddr = MemBase;
+    }
     return RealAddr;
   }
 
@@ -1213,15 +1240,12 @@ void WasmTranslator::translate(
     }
 
     // Pad the rest with zeros
-    SizeT DataSize = Module->min_mem_pages * (64 << 10);
+    SizeT DataSize = Module->min_mem_pages * WASM_PAGE_SIZE;
     if (WritePtr < DataSize) {
       WasmMemory->addInitializer(VariableDeclaration::ZeroInitializer::create(
           Globals.get(), DataSize - WritePtr));
     }
 
-    WasmMemory->addInitializer(VariableDeclaration::ZeroInitializer::create(
-        Globals.get(), Module->min_mem_pages * (64 << 10)));
-
     Globals->push_back(WasmMemory);
 
     lowerGlobals(std::move(Globals));