Subzero WASM: avoid needless comparisons, add bounds check flag option

src/WasmTranslator.cpp

 //===- subzero/src/WasmTranslator.cpp - WASM to Subzero Translation -------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 375 matching lines @@
   Node Float64Constant(double Value) {
     LOG(out << "Float64Constant(" << Value << ") = ");
     auto *Const = Ctx->getConstantDouble(Value);
     assert(Const);
     LOG(out << Node(Const) << "\n");
     return OperandNode(Const);
   }
   Node Binop(wasm::WasmOpcode Opcode, Node Left, Node Right) {
     LOG(out << "Binop(" << WasmOpcodes::OpcodeName(Opcode) << ", " << Left
             << ", " << Right << ") = ");
-    auto *Dest = makeVariable(
-        isComparison(Opcode) ? IceType_i32 : Left.toOperand()->getType());
+    auto *Dest = isComparison(Opcode)

[Jim Stichnoth, 2016/05/06 20:37:51]

I'm surprised "auto" works here, given "bool ? BooleanVariable* : Variable*".

Maybe make it Variable instead?

Also, I think it might be cleaner to use two variables:
  Variable *Dest;
  BooleanVariable *BoolDest;
and avoid all the llvm::cast<BooleanVariable> below.

[Eric Holk, 2016/05/06 20:56:21]

Now that you mention it, I guess I'm a little surprised too.

That said, I like having separate variables better. Dynamic casts make me
nervous.

Done.

+                     ? makeVariable<BooleanVariable>(IceType_i32)
+                     : makeVariable(Left.toOperand()->getType());
     switch (Opcode) {
     case kExprI32Add:
     case kExprI64Add:
       Control()->appendInst(
           InstArithmetic::create(Func, InstArithmetic::Add, Dest, Left, Right));
       break;
     case kExprF32Add:
     case kExprF64Add:
       Control()->appendInst(InstArithmetic::create(Func, InstArithmetic::Fadd,
                                                    Dest, Left, Right));
@@ skipping 123 matching lines @@
     case kExprI32And:
     case kExprI64And:
       Control()->appendInst(
           InstArithmetic::create(Func, InstArithmetic::And, Dest, Left, Right));
       break;
     case kExprI32Ne:
     case kExprI64Ne: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ne, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32Eq:
     case kExprI64Eq: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Eq, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LtS:
     case kExprI64LtS: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Slt, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LeS:
     case kExprI64LeS: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sle, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GeU:
     case kExprI64GeU: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Uge, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LeU:
     case kExprI64LeU: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ule, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32LtU:
     case kExprI64LtU: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ult, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GeS:
     case kExprI64GeS: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sge, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GtS:
     case kExprI64GtS: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Sgt, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprI32GtU:
     case kExprI64GtU: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstIcmp::create(Func, InstIcmp::Ugt, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Eq:
     case kExprF64Eq: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Ueq, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Ne:
     case kExprF64Ne: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Une, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Le:
     case kExprF64Le: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Ule, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Lt:
     case kExprF64Lt: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Ult, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Ge:
     case kExprF64Ge: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Uge, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     case kExprF32Gt:
     case kExprF64Gt: {
       auto *TmpDest = makeVariable(IceType_i1);
       Control()->appendInst(
           InstFcmp::create(Func, InstFcmp::Ugt, TmpDest, Left, Right));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(TmpDest);
       Control()->appendInst(
           InstCast::create(Func, InstCast::Zext, Dest, TmpDest));
       break;
     }
     default:
       LOG(out << "Unknown binop: " << WasmOpcodes::OpcodeName(Opcode) << "\n");
       llvm::report_fatal_error("Uncovered or invalid binop.");
       return OperandNode(nullptr);
     }
     LOG(out << Dest << "\n");
     return OperandNode(Dest);
   }
   Node Unop(wasm::WasmOpcode Opcode, Node Input) {
     LOG(out << "Unop(" << WasmOpcodes::OpcodeName(Opcode) << ", " << Input
             << ") = ");
     Ice::Variable *Dest = nullptr;
     switch (Opcode) {
     // TODO (eholk): merge these next two cases using getConstantInteger
     case kExprI32Eqz: {
-      Dest = makeVariable(IceType_i32);
+      Dest = makeVariable<BooleanVariable>(IceType_i32);
       auto *Tmp = makeVariable(IceType_i1);
       Control()->appendInst(InstIcmp::create(Func, InstIcmp::Eq, Tmp, Input,
                                              Ctx->getConstantInt32(0)));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(Tmp);
       Control()->appendInst(InstCast::create(Func, InstCast::Zext, Dest, Tmp));
       break;
     }
     case kExprI64Eqz: {
-      Dest = makeVariable(IceType_i32);
+      Dest = makeVariable<BooleanVariable>(IceType_i32);
       auto *Tmp = makeVariable(IceType_i1);
       Control()->appendInst(InstIcmp::create(Func, InstIcmp::Eq, Tmp, Input,
                                              Ctx->getConstantInt64(0)));
+      llvm::cast<BooleanVariable>(Dest)->setBoolSource(Tmp);
       Control()->appendInst(InstCast::create(Func, InstCast::Zext, Dest, Tmp));
       break;
     }
     case kExprI32Ctz: {
       Dest = makeVariable(IceType_i32);
       const auto FnName = Ctx->getGlobalString("llvm.cttz.i32");
       bool BadInstrinsic = false;
       const auto *Info = Ctx->getIntrinsicsInfo().find(FnName, BadInstrinsic);
       assert(!BadInstrinsic);
       assert(Info);
@@ skipping 224 matching lines @@
 
     // save control here because true_node appears to alias control.
     auto *Ctrl = Control();
 
     *TrueNode = OperandNode(Func->makeNode());
     *FalseNode = OperandNode(Func->makeNode());
 
     LOG(out << *TrueNode << ", " << *FalseNode << ")"
             << "\n");
 
-    auto *CondBool = makeVariable(IceType_i1);
-    Ctrl->appendInst(InstIcmp::create(Func, InstIcmp::Ne, CondBool, Cond,
-                                      Ctx->getConstantInt32(0)));
+    auto *CondBool = Cond.toOperand()->asBoolean();
+    if (!CondBool) {
+      CondBool = makeVariable(IceType_i1);
+      Ctrl->appendInst(InstIcmp::create(Func, InstIcmp::Ne, CondBool, Cond,
+                                        Ctx->getConstantInt32(0)));
+    }
 
     Ctrl->appendInst(InstBr::create(Func, CondBool, *TrueNode, *FalseNode));
     return OperandNode(nullptr);
   }
   InstSwitch *CurrentSwitch = nullptr;
   CfgNode *SwitchNode = nullptr;
   SizeT SwitchIndex = 0;
   Node Switch(uint32_t Count, Node Key) {
     LOG(out << "Switch(" << Count << ", " << Key << ")\n");
 
@@ skipping 307 matching lines @@
       return nullptr;
     }
     return Iter->second;
   }
 
   void setDefiningInst(Operand *Op, InstPhi *Phi) {
     LOG(out << "\n== setDefiningInst(" << Op << ", " << Phi << ") ==\n");
     PhiMap.emplace(Op, Phi);
   }
 
-  Ice::Variable *makeVariable(Ice::Type Type) {
-    return makeVariable(Type, Control());
+  template <typename T = Ice::Variable> T *makeVariable(Ice::Type Type) {
+    return makeVariable<T>(Type, Control());
   }
 
-  Ice::Variable *makeVariable(Ice::Type Type, CfgNode *DefNode) {
-    auto *Var = Func->makeVariable(Type);
+  template <typename T = Ice::Variable>
+  T *makeVariable(Ice::Type Type, CfgNode *DefNode) {
+    auto *Var = Func->makeVariable<T>(Type);
     DefNodeMap.emplace(Var, DefNode);
     return Var;
   }
 
   CfgNode *getDefNode(Operand *Op) const {
     const auto &Iter = DefNodeMap.find(Op);
     if (Iter == DefNodeMap.end()) {
       return nullptr;
     }
     return Iter->second;
@@ skipping 56 matching lines @@
       ConstZeroBase = (0 == RealOffset);
     } else if (0 != Offset) {
       auto *Addr = makeVariable(Ice::getPointerType());
       auto *OffsetConstant = Ctx->getConstantInt32(Offset);
       Control()->appendInst(InstArithmetic::create(Func, InstArithmetic::Add,
                                                    Addr, Base, OffsetConstant));
 
       Base = Addr;
     }
 
-    // Do the bounds check.
-    //
-    // TODO (eholk): Add a command line argument to control whether bounds
-    // checks are inserted, and maybe add a way to duplicate bounds checks to
-    // get a better sense of the overhead.
-    if (!llvm::dyn_cast<ConstantInteger32>(Base)) {
+    // Do the bounds check if enabled
+    if (!getFlags().getWasmNoBoundsCheck() &&
+        !llvm::dyn_cast<ConstantInteger32>(Base)) {

[Jim Stichnoth, 2016/05/06 20:37:51]

Use isa<> instead of !dyn_cast<> here.

[Eric Holk, 2016/05/06 20:56:21]

Done.

       // TODO (eholk): creating a new basic block on every memory access is
       // terrible (see https://goo.gl/Zj7DTr). Try adding a new instruction that
       // encapsulates this "abort if false" pattern.
       auto *CheckPassed = Func->makeNode();
       auto *CheckFailed = getBoundsFailTarget();
 
       auto *Check = makeVariable(IceType_i1);
       Control()->appendInst(InstIcmp::create(Func, InstIcmp::Ult, Check, Base,
                                              Ctx->getConstantInt32(MemSize)));
       Control()->appendInst(
@@ skipping 235 matching lines @@
       Func->setFunctionName(Ctx->getGlobalString(FnName));
     }
     Ctx->optQueueBlockingPush(makeUnique<CfgOptWorkItem>(std::move(Func)));
     LOG(out << "done.\n");
   }
 
   return;
 }
 
 #endif // ALLOW_WASM