Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WASM/AsmJs: Fix bad cse between signed/unsigned div&rem #3019

Merged
merged 7 commits into from
Jun 8, 2017
Merged

WASM/AsmJs: Fix bad cse between signed/unsigned div&rem #3019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d9903ea
Remove Mul_UInt in asm.js as it is unused and there is no difference …
Cellule May 22, 2017
6545624
Add failing test cases in asm.js and reenable cse on wasm\unsigned.js
Cellule May 22, 2017
587f8fb
Cleanup Div/Rem implementations for Asm.js and Wasm.
Cellule May 22, 2017
9d68f5c
Create DivU_I4 and RemU_I4 to differentiate between signed and unsign…
Cellule May 22, 2017
327de58
Rename Div/Rem _Check_ to _Trap_ and name int64 div/rem with _Trap_
Cellule May 22, 2017
dd1b9f4
Fix div/rem helpers
Cellule May 23, 2017
b1ff34f
Handle overflow for signed rem in interpreter
Cellule May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions lib/Backend/GlobOpt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8273,7 +8273,6 @@ GlobOpt::TypeSpecializeBinary(IR::Instr **pInstr, Value **pSrc1Val, Value **pSrc
IR::Instr *&instr = *pInstr;
int32 min1 = INT32_MIN, max1 = INT32_MAX, min2 = INT32_MIN, max2 = INT32_MAX, newMin, newMax, tmp;
Js::OpCode opcode;
IR::Opnd *src1, *src2;
Value *&src1Val = *pSrc1Val;
Value *&src2Val = *pSrc2Val;

Expand Down Expand Up @@ -8589,6 +8588,7 @@ GlobOpt::TypeSpecializeBinary(IR::Instr **pInstr, Value **pSrc1Val, Value **pSrc
src2Lossy = false;

opcode = Js::OpCode::Div_I4;
Assert(!instr->GetSrc1()->IsUnsigned());
bailOutKind |= IR::BailOnDivResultNotInt;
if (max2 >= 0 && min2 <= 0)
{
Expand Down Expand Up @@ -9215,7 +9215,7 @@ GlobOpt::TypeSpecializeBinary(IR::Instr **pInstr, Value **pSrc1Val, Value **pSrc
}
case Js::OpCode::Rem_A:
{
src2 = instr->GetSrc2();
IR::Opnd* src2 = instr->GetSrc2();
if (!this->IsLoopPrePass() && min2 == max2 && min1 >= 0)
{
int32 value = min2;
Expand Down Expand Up @@ -9309,6 +9309,7 @@ GlobOpt::TypeSpecializeBinary(IR::Instr **pInstr, Value **pSrc1Val, Value **pSrc
newMax = max(newMin, newMax);
}
opcode = Js::OpCode::Rem_I4;
Assert(!instr->GetSrc1()->IsUnsigned());
break;
}

Expand Down Expand Up @@ -9599,12 +9600,12 @@ GlobOpt::TypeSpecializeBinary(IR::Instr **pInstr, Value **pSrc1Val, Value **pSrc
}

// Make sure the srcs are specialized
src1 = instr->GetSrc1();
IR::Opnd* src1 = instr->GetSrc1();
this->ToInt32(instr, src1, this->currentBlock, src1ValueToSpecialize, nullptr, src1Lossy);

if (!skipSrc2)
{
src2 = instr->GetSrc2();
IR::Opnd* src2 = instr->GetSrc2();
this->ToInt32(instr, src2, this->currentBlock, src2ValueToSpecialize, nullptr, src2Lossy);
}

Expand Down
2 changes: 2 additions & 0 deletions lib/Backend/GlobOptExpr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,9 @@ GlobOpt::CSEAddInstr(
case Js::OpCode::Neg_I4:
case Js::OpCode::Add_I4:
case Js::OpCode::Sub_I4:
case Js::OpCode::DivU_I4:
case Js::OpCode::Div_I4:
case Js::OpCode::RemU_I4:
case Js::OpCode::Rem_I4:
case Js::OpCode::ShrU_I4:
{
Expand Down
25 changes: 4 additions & 21 deletions lib/Backend/IR.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3747,7 +3747,6 @@ template <typename T>
bool Instr::BinaryCalculatorT(T src1Const, T src2Const, int64 *pResult)
{
T value = 0;
bool check = true;
switch (this->m_opcode)
{
#define BINARY_U(OPCODE,HANDLER) \
Expand All @@ -3768,7 +3767,6 @@ bool Instr::BinaryCalculatorT(T src1Const, T src2Const, int64 *pResult)
BINARY_U(CmUnGt_I4, Js::AsmJsMath::CmpGt)
BINARY_U(CmUnLe_I4, Js::AsmJsMath::CmpLe)
BINARY_U(CmUnGe_I4, Js::AsmJsMath::CmpGe)
//
BINARY(Add_I4, Js::AsmJsMath::Add)
BINARY(Sub_I4, Js::AsmJsMath::Sub)
BINARY(Mul_I4, Js::AsmJsMath::Mul)
Expand All @@ -3778,25 +3776,10 @@ bool Instr::BinaryCalculatorT(T src1Const, T src2Const, int64 *pResult)
BINARY(Shl_I4, Wasm::WasmMath::Shl)
BINARY(Shr_I4, Wasm::WasmMath::Shr)
BINARY_U(ShrU_I4, Wasm::WasmMath::ShrU)
case Js::OpCode::Div_I4:
check = GetSrc1()->IsUnsigned() || !(src1Const == SignedTypeTraits<T>::MinValue && src2Const == -1);
case Js::OpCode::Rem_I4:
if (check && (src2Const != 0))
{
if (GetSrc1()->IsUnsigned())
{
value = m_opcode == Js::OpCode::Div_I4 ?
Js::AsmJsMath::Div<typename SignedTypeTraits<T>::UnsignedType>(src1Const, src2Const) :
Js::AsmJsMath::Rem<typename SignedTypeTraits<T>::UnsignedType>(src1Const, src2Const);
}
else
{
value = m_opcode == Js::OpCode::Div_I4 ?
Js::AsmJsMath::Div<T>(src1Const, src2Const) :
Js::AsmJsMath::Rem<T>(src1Const, src2Const);
}
}
break;
BINARY(Div_I4, Js::AsmJsMath::DivChecked)
BINARY_U(DivU_I4, Js::AsmJsMath::DivChecked)
BINARY(Rem_I4, Js::AsmJsMath::RemChecked)
BINARY_U(RemU_I4, Js::AsmJsMath::RemChecked)
default:
return false;
#undef BINARY
Expand Down
43 changes: 25 additions & 18 deletions lib/Backend/IRBuilderAsmJs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2396,41 +2396,42 @@ IRBuilderAsmJs::BuildInt3(Js::OpCodeAsmJs newOpcode, uint32 offset, Js::RegSlot
instr = IR::Instr::New(Js::OpCode::Sub_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;

case Js::OpCodeAsmJs::Mul_UInt:
src1Opnd->SetType(TyUint32);
src2Opnd->SetType(TyUint32);
case Js::OpCodeAsmJs::Mul_Int:
instr = IR::Instr::New(Js::OpCode::Mul_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Div_Check_UInt:
case Js::OpCodeAsmJs::Div_Trap_UInt:
src1Opnd->SetType(TyUint32);
src2Opnd->SetType(TyUint32);
case Js::OpCodeAsmJs::Div_Check_Int:
{
// Fall through for trap
case Js::OpCodeAsmJs::Div_Trap_Int:
src2Opnd = BuildTrapIfZero(src2Opnd, offset);
if (newOpcode == Js::OpCodeAsmJs::Div_Check_Int)
if (newOpcode == Js::OpCodeAsmJs::Div_Trap_Int)
{
src1Opnd = BuildTrapIfMinIntOverNegOne(src1Opnd, src2Opnd, offset);
}
instr = IR::Instr::New(Js::OpCode::Div_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
instr = IR::Instr::New(newOpcode == Js::OpCodeAsmJs::Div_Trap_UInt ? Js::OpCode::DivU_I4 : Js::OpCode::Div_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
}
case Js::OpCodeAsmJs::Div_UInt:
src1Opnd->SetType(TyUint32);
src2Opnd->SetType(TyUint32);
instr = IR::Instr::New(Js::OpCode::DivU_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Div_Int:
instr = IR::Instr::New(Js::OpCode::Div_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Rem_Check_UInt:
case Js::OpCodeAsmJs::Rem_Trap_UInt:
src1Opnd->SetType(TyUint32);
src2Opnd->SetType(TyUint32);
case Js::OpCodeAsmJs::Rem_Check_Int:
// Fall through for trap
case Js::OpCodeAsmJs::Rem_Trap_Int:
src2Opnd = BuildTrapIfZero(src2Opnd, offset);
instr = IR::Instr::New(Js::OpCode::Rem_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
instr = IR::Instr::New(newOpcode == Js::OpCodeAsmJs::Rem_Trap_UInt ? Js::OpCode::RemU_I4 : Js::OpCode::Rem_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Rem_UInt:
src1Opnd->SetType(TyUint32);
src2Opnd->SetType(TyUint32);
instr = IR::Instr::New(Js::OpCode::RemU_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Rem_Int:
instr = IR::Instr::New(Js::OpCode::Rem_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
Expand Down Expand Up @@ -3008,23 +3009,29 @@ IRBuilderAsmJs::BuildLong3(Js::OpCodeAsmJs newOpcode, uint32 offset, Js::RegSlot
case Js::OpCodeAsmJs::Mul_Long:
instr = IR::Instr::New(Js::OpCode::Mul_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
case Js::OpCodeAsmJs::Div_ULong:
case Js::OpCodeAsmJs::Div_Trap_ULong:
src1Opnd->SetType(TyUint64);
src2Opnd->SetType(TyUint64);
case Js::OpCodeAsmJs::Div_Long:
// Fall Through for trap
case Js::OpCodeAsmJs::Div_Trap_Long:
{
src2Opnd = BuildTrapIfZero(src2Opnd, offset);
src1Opnd = BuildTrapIfMinIntOverNegOne(src1Opnd, src2Opnd, offset);
instr = IR::Instr::New(Js::OpCode::Div_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
Js::OpCode op = newOpcode == Js::OpCodeAsmJs::Div_Trap_ULong ? Js::OpCode::DivU_I4 : Js::OpCode::Div_I4;
instr = IR::Instr::New(op, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
}
case Js::OpCodeAsmJs::Rem_ULong:
case Js::OpCodeAsmJs::Rem_Trap_ULong:
src1Opnd->SetType(TyUint64);
src2Opnd->SetType(TyUint64);
case Js::OpCodeAsmJs::Rem_Long:
// Fall Through for trap
case Js::OpCodeAsmJs::Rem_Trap_Long:
{
src2Opnd = BuildTrapIfZero(src2Opnd, offset);
instr = IR::Instr::New(Js::OpCode::Rem_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
Js::OpCode op = newOpcode == Js::OpCodeAsmJs::Rem_Trap_ULong ? Js::OpCode::RemU_I4 : Js::OpCode::Rem_I4;
instr = IR::Instr::New(op, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
}
case Js::OpCodeAsmJs::And_Long:
instr = IR::Instr::New(Js::OpCode::And_I4, dstOpnd, src1Opnd, src2Opnd, m_func);
break;
Expand Down
8 changes: 4 additions & 4 deletions lib/Backend/JnHelperMethodList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,10 +570,10 @@ HELPERCALL(DirectMath_Exp, nullptr, 0)
HELPERCALL(DirectMath_Log, nullptr, 0)
HELPERCALL(DirectMath_Sin, nullptr, 0)
HELPERCALL(DirectMath_Tan, nullptr, 0)
HELPERCALL(DirectMath_Int64DivS, ((int64(*)(int64, int64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_DivOverflow<int64, &Js::AsmJsMath::Div<int64>, LONGLONG_MIN>), AttrCanThrow)
HELPERCALL(DirectMath_Int64DivU, ((uint64(*)(uint64, uint64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_DivRemCheck<uint64, &Js::AsmJsMath::Div<uint64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64RemS, ((int64(*)(int64, int64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_DivRemCheck<int64, &Wasm::WasmMath::Rem<int64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64RemU, ((uint64(*)(uint64, uint64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_DivRemCheck<uint64, &Wasm::WasmMath::Rem<uint64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64DivS, ((int64(*)(int64, int64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_DivOverflow<int64, &Js::AsmJsMath::DivUnsafe<int64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64DivU, ((uint64(*)(uint64, uint64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_UnsignedDivRemCheck<uint64, &Js::AsmJsMath::DivUnsafe<uint64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64RemS, ((int64(*)(int64, int64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_RemOverflow<int64, &Js::AsmJsMath::RemUnsafe<int64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64RemU, ((uint64(*)(uint64, uint64, Js::ScriptContext*)) Js::InterpreterStackFrame::OP_UnsignedDivRemCheck<uint64, &Js::AsmJsMath::RemUnsafe<uint64>>), AttrCanThrow)
HELPERCALL(DirectMath_Int64Mul , (int64(*)(int64,int64)) Js::AsmJsMath::Mul<int64>, 0)
HELPERCALL(DirectMath_Int64Shl , (int64(*)(int64,int64)) Wasm::WasmMath::Shl<int64>, 0)
HELPERCALL(DirectMath_Int64Shr , (int64(*)(int64,int64)) Wasm::WasmMath::Shr<int64>, 0)
Expand Down
20 changes: 12 additions & 8 deletions lib/Backend/Lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,7 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
case Js::OpCode::Add_I4:
case Js::OpCode::Sub_I4:
case Js::OpCode::Mul_I4:
case Js::OpCode::RemU_I4:
case Js::OpCode::Rem_I4:
case Js::OpCode::Or_I4:
case Js::OpCode::Xor_I4:
Expand Down Expand Up @@ -1091,7 +1092,7 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
}
else
{
if (instr->m_opcode == Js::OpCode::Rem_I4)
if (instr->m_opcode == Js::OpCode::Rem_I4 || instr->m_opcode == Js::OpCode::RemU_I4)
{
// fast path
this->GenerateSimplifiedInt4Rem(instr);
Expand Down Expand Up @@ -1120,6 +1121,7 @@ Lowerer::LowerRange(IR::Instr *instrStart, IR::Instr *instrEnd, bool defaultDoFa
instr->m_opcode = Js::OpCode::Ld_I4; //to simplify handling of i32/i64
instrPrev = instr; //re-evaluate -- let Ld_I4 handler to properly lower the operand.
break;
case Js::OpCode::DivU_I4:
case Js::OpCode::Div_I4:
this->LowerDivI4(instr);
break;
Expand Down Expand Up @@ -23482,7 +23484,7 @@ Lowerer::GenerateSimplifiedInt4Rem(
IR::LabelInstr *const skipBailOutLabel) const
{
Assert(remInstr);
Assert(remInstr->m_opcode == Js::OpCode::Rem_I4);
Assert(remInstr->m_opcode == Js::OpCode::Rem_I4 || remInstr->m_opcode == Js::OpCode::RemU_I4);

auto *dst = remInstr->GetDst(), *src1 = remInstr->GetSrc1(), *src2 = remInstr->GetSrc2();

Expand Down Expand Up @@ -23851,9 +23853,11 @@ void
Lowerer::LowerDivI4Common(IR::Instr * instr)
{
Assert(instr);
Assert(instr->m_opcode == Js::OpCode::Rem_I4 || instr->m_opcode == Js::OpCode::Div_I4);
Assert((instr->m_opcode == Js::OpCode::Rem_I4 || instr->m_opcode == Js::OpCode::Div_I4) ||
(instr->m_opcode == Js::OpCode::RemU_I4 || instr->m_opcode == Js::OpCode::DivU_I4));
Assert(m_func->GetJITFunctionBody()->IsAsmJsMode());

const bool isRem = instr->m_opcode == Js::OpCode::Rem_I4 || instr->m_opcode == Js::OpCode::RemU_I4;
// MIN_INT/-1 path is only needed for signed operations

// TEST src2, src2
Expand Down Expand Up @@ -23896,7 +23900,7 @@ Lowerer::LowerDivI4Common(IR::Instr * instr)
{
InsertMove(dst, IR::IntConstOpnd::NewFromType(0, dst->GetType(), m_func), divLabel);
}
InsertBranch(Js::OpCode::Br, doneLabel, divLabel);
InsertBranch(Js::OpCode::Br, doneLabel, divLabel);
}


Expand All @@ -23907,7 +23911,7 @@ Lowerer::LowerDivI4Common(IR::Instr * instr)
int64 intMin = IRType_IsInt64(src1->GetType()) ? LONGLONG_MIN : INT_MIN;
IR::Instr* overflowReg3 = IR::Instr::FindSingleDefInstr(Js::OpCode::TrapIfMinIntOverNegOne, instr->GetSrc1());
bool needsMinOverNeg1Check = true;
if (isWasmFunc && instr->m_opcode != Js::OpCode::Rem_I4)
if (isWasmFunc && !isRem)
{
needsMinOverNeg1Check = overflowReg3 != nullptr;
}
Expand Down Expand Up @@ -23947,7 +23951,7 @@ Lowerer::LowerDivI4Common(IR::Instr * instr)
}
else
{
InsertMove(dst, instr->m_opcode == Js::OpCode::Div_I4 ? src1 : IR::IntConstOpnd::NewFromType(0, dst->GetType(), m_func), divLabel);
InsertMove(dst, !isRem ? src1 : IR::IntConstOpnd::NewFromType(0, dst->GetType(), m_func), divLabel);
}
InsertBranch(Js::OpCode::Br, doneLabel, divLabel);
}
Expand All @@ -23961,7 +23965,7 @@ void
Lowerer::LowerRemI4(IR::Instr * instr)
{
Assert(instr);
Assert(instr->m_opcode == Js::OpCode::Rem_I4);
Assert(instr->m_opcode == Js::OpCode::Rem_I4 || instr->m_opcode == Js::OpCode::RemU_I4);
if (m_func->GetJITFunctionBody()->IsAsmJsMode())
{
LowerDivI4Common(instr);
Expand All @@ -23987,7 +23991,7 @@ void
Lowerer::LowerDivI4(IR::Instr * instr)
{
Assert(instr);
Assert(instr->m_opcode == Js::OpCode::Div_I4);
Assert(instr->m_opcode == Js::OpCode::Div_I4 || instr->m_opcode == Js::OpCode::DivU_I4);

#ifdef _M_IX86
if (
Expand Down
3 changes: 3 additions & 0 deletions lib/Backend/amd64/LowererMDArch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2239,9 +2239,11 @@ LowererMDArch::EmitInt4Instr(IR::Instr *instr, bool signExtend /* = false */)
legalize = true;
break;

case Js::OpCode::DivU_I4:
case Js::OpCode::Div_I4:
instr->SinkDst(Js::OpCode::MOV, RegRAX);
goto idiv_common;
case Js::OpCode::RemU_I4:
case Js::OpCode::Rem_I4:
instr->SinkDst(Js::OpCode::MOV, RegRDX);
idiv_common:
Expand All @@ -2250,6 +2252,7 @@ LowererMDArch::EmitInt4Instr(IR::Instr *instr, bool signExtend /* = false */)
if (isUnsigned)
{
Assert(instr->GetSrc2()->IsUnsigned());
Assert(instr->m_opcode == Js::OpCode::RemU_I4 || instr->m_opcode == Js::OpCode::DivU_I4);
instr->m_opcode = Js::OpCode::DIV;
}
else
Expand Down
4 changes: 4 additions & 0 deletions lib/Backend/arm/LowerMD.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6445,10 +6445,14 @@ LowererMD::EmitInt4Instr(IR::Instr *instr)
instr->m_opcode = Js::OpCode::MUL;
break;

case Js::OpCode::DivU_I4:
AssertMsg(UNREACHED, "Unsigned div NYI");
case Js::OpCode::Div_I4:
instr->m_opcode = Js::OpCode::SDIV;
break;

case Js::OpCode::RemU_I4:
AssertMsg(UNREACHED, "Unsigned rem NYI");
case Js::OpCode::Rem_I4:
instr->m_opcode = Js::OpCode::REM;
break;
Expand Down
Loading