From 745773fd7492dfe3718a93bf97ea381618fb9bbc Mon Sep 17 00:00:00 2001 From: Owen Yamauchi Date: Tue, 14 May 2013 15:28:12 -0700 Subject: [PATCH] Hack-fix logical op types This badly needs a proper rewrite, but this is intended to be a low-risk, mergeable change. These ops' destination type is always marked as Int (wrong), which meant that the "zero extend if bool" logic was never invoked. Logical ops always output Bool. Once we fix this, we'll save some instructions downstream, because right now the Xor (etc.) spend an instruction zero-extending their result, only for the next instruction to be (usually) ConvIntToBool. Also discovered some mildly unsettling "&" vs. "&&" issues. --- hphp/runtime/vm/translator/hopt/codegen.cpp | 48 +++++++++++---------- hphp/runtime/vm/translator/hopt/codegen.h | 4 +- hphp/runtime/vm/translator/hopt/ir.h | 2 + hphp/test/slow/types/1751.php | 10 +++++ hphp/test/slow/types/1751.php.expect | 3 ++ 5 files changed, 42 insertions(+), 25 deletions(-) create mode 100644 hphp/test/slow/types/1751.php create mode 100644 hphp/test/slow/types/1751.php.expect diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp index e4fdf4b6e..8719c12bc 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.cpp +++ b/hphp/runtime/vm/translator/hopt/codegen.cpp @@ -516,8 +516,7 @@ void shuffle2(CodeGenerator::Asm& a, } } -static void signExtendBool(X64Assembler& as, const SSATmp* tmp, - const RegisterInfo& info) { +static void signExtendBool(X64Assembler& as, const RegisterInfo& info) { auto reg = info.getReg(); if (reg != InvalidReg) { // sign-extend the bool from a byte to a quad @@ -525,15 +524,19 @@ static void signExtendBool(X64Assembler& as, const SSATmp* tmp, } } +static void zeroExtendBool(X64Assembler& as, const RegisterInfo& info) { + auto reg = info.getReg(); + if (reg != InvalidReg) { + // zero-extend the bool from a byte to a quad + // note: movzbl actually extends the value to 64 bits. + as.movzbl(rbyte(reg), r32(reg)); + } +} + static void zeroExtendIfBool(X64Assembler& as, const SSATmp* src, const RegisterInfo& info) { if (src->isA(Type::Bool)) { - auto reg = info.getReg(); - if (reg != InvalidReg) { - // zero-extend the bool from a byte to a quad - // note: movzbl actually extends the value to 64 bits. - as.movzbl(rbyte(reg), r32(reg)); - } + zeroExtendBool(as, info); } } @@ -979,7 +982,7 @@ void CodeGenerator::cgBinaryIntOp(IRInstruction* inst, void (Asm::*instrIR)(Immed, RegType), void (Asm::*instrRR)(RegType, RegType), void (Asm::*movInstr)(RegType, RegType), - void (*extendInstr)(Asm&, const SSATmp*, const RegisterInfo&), + void (*extendInstr)(Asm&, const RegisterInfo&), Oper oper, RegType (*convertReg)(PhysReg), Commutativity commuteFlag) { @@ -1014,7 +1017,7 @@ void CodeGenerator::cgBinaryIntOp(IRInstruction* inst, auto signExtendIfNecessary = [&] { if (useBoolOps) { - extendInstr(m_as, dst, m_regs[dst]); + extendInstr(m_as, m_regs[dst]); } }; @@ -1093,7 +1096,7 @@ void CodeGenerator::cgBinaryOp(IRInstruction* inst, void (Asm::*instrRR)(RegType, RegType), void (Asm::*movInstr)(RegType, RegType), void (Asm::*fpInstr)(RegXMM, RegXMM), - void (*extendInstr)(Asm&, const SSATmp*, const RegisterInfo&), + void (*extendInstr)(Asm&, const RegisterInfo&), Oper oper, RegType (*convertReg)(PhysReg), Commutativity commuteFlag) { @@ -1154,13 +1157,12 @@ void CodeGenerator::cgOpAdd(IRInstruction* inst) { // Special cases: x = y + 1 if (emitInc(dst, src1, src2) || emitInc(dst, src2, src1)) return; - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) - { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::addb, &Asm::addb, &Asm::movb, - &zeroExtendIfBool, + &zeroExtendBool, std::plus(), &convertToReg8, Commutative); @@ -1189,7 +1191,7 @@ void CodeGenerator::cgOpSub(IRInstruction* inst) { return cgNegateWork(dst, src2); } - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::subb, &Asm::subb, @@ -1215,12 +1217,12 @@ void CodeGenerator::cgOpAnd(IRInstruction* inst) { SSATmp* src1 = inst->getSrc(0); SSATmp* src2 = inst->getSrc(1); - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::andb, &Asm::andb, &Asm::movb, - &zeroExtendIfBool, + &zeroExtendBool, [] (bool a, bool b) { return a & b; }, &convertToReg8, Commutative); @@ -1240,12 +1242,12 @@ void CodeGenerator::cgOpOr(IRInstruction* inst) { SSATmp* src1 = inst->getSrc(0); SSATmp* src2 = inst->getSrc(1); - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::orb, &Asm::orb, &Asm::movb, - &zeroExtendIfBool, + &zeroExtendBool, [] (bool a, bool b) { return a | b; }, &convertToReg8, Commutative); @@ -1274,12 +1276,12 @@ void CodeGenerator::cgOpXor(IRInstruction* inst) { return cgNotWork(dst, src1); } - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::xorb, &Asm::xorb, &Asm::movb, - &zeroExtendIfBool, + &zeroExtendBool, [] (bool a, bool b) { return a ^ b; }, &convertToReg8, Commutative); @@ -1299,12 +1301,12 @@ void CodeGenerator::cgOpMul(IRInstruction* inst) { SSATmp* src1 = inst->getSrc(0); SSATmp* src2 = inst->getSrc(1); - if (src1->isA(Type::Bool) & src2->isA(Type::Bool)) { + if (src1->isA(Type::Bool) && src2->isA(Type::Bool)) { cgBinaryIntOp(inst, &Asm::andb, &Asm::andb, &Asm::movb, - &zeroExtendIfBool, + &zeroExtendBool, [] (bool a, bool b) { return a & b; }, &convertToReg8, Commutative); diff --git a/hphp/runtime/vm/translator/hopt/codegen.h b/hphp/runtime/vm/translator/hopt/codegen.h index 8e6454e33..176938806 100644 --- a/hphp/runtime/vm/translator/hopt/codegen.h +++ b/hphp/runtime/vm/translator/hopt/codegen.h @@ -212,7 +212,7 @@ private: void (Asm::*intRR)(RegType, RegType), void (Asm::*mov)(RegType, RegType), void (Asm::*fpRR)(RegXMM, RegXMM), - void (*extend)(Asm&, const SSATmp*, const RegisterInfo&), + void (*extend)(Asm&, const RegisterInfo&), Oper, RegType (*conv)(PhysReg), Commutativity); @@ -221,7 +221,7 @@ private: void (Asm::*intImm)(Immed, RegType), void (Asm::*intRR)(RegType, RegType), void (Asm::*mov)(RegType, RegType), - void (*extend)(Asm&, const SSATmp*, const RegisterInfo&), + void (*extend)(Asm&, const RegisterInfo&), Oper, RegType (*conv)(PhysReg), Commutativity); diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h index 6e1ad46d2..14dbd7599 100644 --- a/hphp/runtime/vm/translator/hopt/ir.h +++ b/hphp/runtime/vm/translator/hopt/ir.h @@ -593,6 +593,8 @@ enum Opcode : uint16_t { #undef O // Agree with hhbc on the names of these, to ease macro implementations. + // TODO(2395841): this is totally bogus. Bitwise and logical ops have vastly + // different behavior. We need to give these their own opcodes. OpBitAnd = OpAnd, OpBitOr = OpOr, OpBitXor = OpXor, diff --git a/hphp/test/slow/types/1751.php b/hphp/test/slow/types/1751.php new file mode 100644 index 000000000..df44cada1 --- /dev/null +++ b/hphp/test/slow/types/1751.php @@ -0,0 +1,10 @@ +