src/WasmTranslator.cpp - Issue 1961583002: Subzero WASM: avoid needless comparisons, add bounds check flag option

Unified Diff: src/WasmTranslator.cpp

Issue 1961583002: Subzero WASM: avoid needless comparisons, add bounds check flag option (Closed) Base URL: https://chromium.googlesource.com/native_client/pnacl-subzero.git@master

Patch Set: Add BooleanVariable::classof Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: src/WasmTranslator.cpp

diff --git a/src/WasmTranslator.cpp b/src/WasmTranslator.cpp

index 792e134cb2c3508f0ca04cff536d401f3d17f1cc..19c106d5254f69e041df649ff156e6a60a2ed018 100644

--- a/src/WasmTranslator.cpp

+++ b/src/WasmTranslator.cpp

@@ -393,8 +393,9 @@ public:

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 ? Boo

Eric Holk 2016/05/06 20:56:21 Now that you mention it, I guess I'm a little surp

+ ? makeVariable<BooleanVariable>(IceType_i32)

+ : makeVariable(Left.toOperand()->getType());

switch (Opcode) {

case kExprI32Add:

case kExprI64Add:

@@ -538,6 +539,7 @@ public:

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;

@@ -547,6 +549,7 @@ public:

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;

@@ -556,6 +559,7 @@ public:

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;

@@ -565,6 +569,7 @@ public:

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;

@@ -574,6 +579,7 @@ public:

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;

@@ -583,6 +589,7 @@ public:

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;

@@ -592,6 +599,7 @@ public:

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;

@@ -601,6 +609,7 @@ public:

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;

@@ -610,6 +619,7 @@ public:

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;

@@ -619,6 +629,7 @@ public:

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;

@@ -628,6 +639,7 @@ public:

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;

@@ -637,6 +649,7 @@ public:

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;

@@ -646,6 +659,7 @@ public:

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;

@@ -655,6 +669,7 @@ public:

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;

@@ -664,6 +679,7 @@ public:

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;

@@ -673,6 +689,7 @@ public:

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;

@@ -692,18 +709,20 @@ public:

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;

}

@@ -948,9 +967,12 @@ public:

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);

@@ -1278,12 +1300,13 @@ private:

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;

}

@@ -1360,12 +1383,9 @@ private:

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.

« src/IceClFlags.def ('K') | « src/IceOperand.h ('k') | no next file » | no next file with comments »