From d5cb85816593fb494808671f8a0d6481db538b9b Mon Sep 17 00:00:00 2001 From: Sintendo Date: Sun, 12 Jan 2020 22:45:34 +0100 Subject: [PATCH 1/5] x64Emitter: Unit test memory addressing modes Test the behavior of OpArg::WriteRest by using MOV with the various addressing modes (MatR, MRegSum, etc.) in the source operand. Both the instruction and the instruction length are validated. --- Source/UnitTests/Common/x64EmitterTest.cpp | 91 ++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/Source/UnitTests/Common/x64EmitterTest.cpp b/Source/UnitTests/Common/x64EmitterTest.cpp index dedac29df3..77006f211c 100644 --- a/Source/UnitTests/Common/x64EmitterTest.cpp +++ b/Source/UnitTests/Common/x64EmitterTest.cpp @@ -576,6 +576,97 @@ TEST_F(x64EmitterTest, MOV64) } } +TEST_F(x64EmitterTest, MOV_AtReg) +{ + for (const auto& src : reg64names) + { + std::string segment = src.reg == RSP || src.reg == RBP ? "ss" : "ds"; + + emitter->MOV(64, R(RAX), MatR(src.reg)); + EXPECT_EQ(emitter->GetCodePtr(), + code_buffer + 3 + ((src.reg & 7) == RBP || (src.reg & 7) == RSP)); + ExpectDisassembly("mov rax, qword ptr " + segment + ":[" + src.name + "]"); + } +} + +TEST_F(x64EmitterTest, MOV_RegSum) +{ + for (const auto& src2 : reg64names) + { + for (const auto& src1 : reg64names) + { + if (src2.reg == RSP) + continue; + std::string segment = src1.reg == RSP || src1.reg == RBP ? "ss" : "ds"; + + emitter->MOV(64, R(RAX), MRegSum(src1.reg, src2.reg)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 4 + ((src1.reg & 7) == RBP)); + ExpectDisassembly("mov rax, qword ptr " + segment + ":[" + src1.name + "+" + src2.name + "]"); + } + } +} + +TEST_F(x64EmitterTest, MOV_Disp) +{ + for (const auto& dest : reg64names) + { + for (const auto& src : reg64names) + { + std::string segment = src.reg == RSP || src.reg == RBP ? "ss" : "ds"; + + emitter->MOV(64, R(dest.reg), MDisp(src.reg, 42)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 4 + ((src.reg & 7) == RSP)); + ExpectDisassembly("mov " + dest.name + ", qword ptr " + segment + ":[" + src.name + "+42]"); + + emitter->MOV(64, R(dest.reg), MDisp(src.reg, 1000)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 7 + ((src.reg & 7) == RSP)); + ExpectDisassembly("mov " + dest.name + ", qword ptr " + segment + ":[" + src.name + "+1000]"); + } + } +} + +TEST_F(x64EmitterTest, MOV_Scaled) +{ + for (const auto& src : reg64names) + { + if (src.reg == RSP) + continue; + + emitter->MOV(64, R(RAX), MScaled(src.reg, 2, 42)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 8); + ExpectDisassembly("mov rax, qword ptr ds:[" + src.name + "*2+42]"); + } +} + +TEST_F(x64EmitterTest, MOV_Complex) +{ + for (const auto& src1 : reg64names) + { + std::string segment = src1.reg == RSP || src1.reg == RBP ? "ss" : "ds"; + + for (const auto& src2 : reg64names) + { + if (src2.reg == RSP) + continue; + + emitter->MOV(64, R(RAX), MComplex(src1.reg, src2.reg, 4, 0)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 4 + ((src1.reg & 7) == RBP)); + ExpectDisassembly("mov rax, qword ptr " + segment + ":[" + src1.name + "+" + src2.name + + "*4]"); + + emitter->MOV(64, R(RAX), MComplex(src1.reg, src2.reg, 4, 42)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 5); + ExpectDisassembly("mov rax, qword ptr " + segment + ":[" + src1.name + "+" + src2.name + + "*4+42]"); + + emitter->MOV(64, R(RAX), MComplex(src1.reg, src2.reg, 4, 1000)); + EXPECT_EQ(emitter->GetCodePtr(), code_buffer + 8); + ExpectDisassembly("mov rax, qword ptr " + segment + ":[" + src1.name + "+" + src2.name + + "*4+1000]"); + } + } +} + // TODO: Disassembler inverts operands here. // TWO_OP_ARITH_TEST(XCHG) // TWO_OP_ARITH_TEST(TEST) From cde3a3b44865d6654884f7c9be691fdf64bc9927 Mon Sep 17 00:00:00 2001 From: Sintendo Date: Sun, 12 Jan 2020 23:48:16 +0100 Subject: [PATCH 2/5] x64Emitter: Avoid 8-bit displacement when possible Due to the way the ModRM encoding works on x86, memory addressing combinations involving RBP or R13 need an additional byte for an 8-bit displacement of zero. However, this was also applied in cases where it is unnecessary, effectively wasting a byte. - MatR with RSP or R12 8B 44 24 00 mov eax,dword ptr [rsp] 8B 04 24 mov eax,dword ptr [rsp] - MRegSum with base != RBP or R13 46 8D 7C 37 00 lea r15d,[rdi+r14] 46 8D 3C 37 lea r15d,[rdi+r14] - MComplex without offset 8B 4C CA 00 mov ecx,dword ptr [rdx+rcx*8] 8B 0C CA mov ecx,dword ptr [rdx+rcx*8] --- Source/Core/Common/x64Emitter.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Source/Core/Common/x64Emitter.cpp b/Source/Core/Common/x64Emitter.cpp index 947e6e5614..169ffdc519 100644 --- a/Source/Core/Common/x64Emitter.cpp +++ b/Source/Core/Common/x64Emitter.cpp @@ -323,7 +323,11 @@ void OpArg::WriteRest(XEmitter* emit, int extraBytes, X64Reg _operandReg, // Okay, we're fine. Just disp encoding. // We need displacement. Which size? int ioff = (int)(s64)offset; - if (ioff < -128 || ioff > 127) + if (ioff == 0 && (_offsetOrBaseReg & 7) != 5) + { + mod = 0; + } + else if (ioff < -128 || ioff > 127) { mod = 2; // 32-bit displacement } From bdfc472751caad9843b6527141264550df80bb6d Mon Sep 17 00:00:00 2001 From: Sintendo Date: Mon, 13 Jan 2020 00:10:46 +0100 Subject: [PATCH 3/5] x64Emitter: Refactor OpArg::WriteRest Shorter, displacement is now handled in one location. --- Source/Core/Common/x64Emitter.cpp | 82 +++++++++++-------------------- 1 file changed, 28 insertions(+), 54 deletions(-) diff --git a/Source/Core/Common/x64Emitter.cpp b/Source/Core/Common/x64Emitter.cpp index 169ffdc519..a90af4c288 100644 --- a/Source/Core/Common/x64Emitter.cpp +++ b/Source/Core/Common/x64Emitter.cpp @@ -270,71 +270,45 @@ void OpArg::WriteRest(XEmitter* emit, int extraBytes, X64Reg _operandReg, return; } - if (scale == 0) + if (scale == SCALE_NONE) { // Oh, no memory, Just a reg. mod = 3; // 11 } + else if (scale >= SCALE_NOBASE_2 && scale <= SCALE_NOBASE_8) + { + SIB = true; + mod = 0; + _offsetOrBaseReg = 5; + // Always has 32-bit displacement + } else { - // Ah good, no scaling. - if (scale == SCALE_ATREG && !((_offsetOrBaseReg & 7) == 4 || (_offsetOrBaseReg & 7) == 5)) - { - // Okay, we're good. No SIB necessary. - int ioff = (int)offset; - if (ioff == 0) - { - mod = 0; - } - else if (ioff < -128 || ioff > 127) - { - mod = 2; // 32-bit displacement - } - else - { - mod = 1; // 8-bit displacement - } - } - else if (scale >= SCALE_NOBASE_2 && scale <= SCALE_NOBASE_8) + if (scale != SCALE_ATREG) { SIB = true; - mod = 0; - _offsetOrBaseReg = 5; + } + else if ((_offsetOrBaseReg & 7) == 4) + { + // Special case for which SCALE_ATREG needs SIB + SIB = true; + ireg = _offsetOrBaseReg; + } + + // Okay, we're fine. Just disp encoding. + // We need displacement. Which size? + int ioff = (int)(s64)offset; + if (ioff == 0 && (_offsetOrBaseReg & 7) != 5) + { + mod = 0; // No displacement + } + else if (ioff >= -128 && ioff <= 127) + { + mod = 1; // 8-bit displacement } else { - if ((_offsetOrBaseReg & 7) == 4) // this would occupy the SIB encoding :( - { - // So we have to fake it with SIB encoding :( - SIB = true; - } - - if (scale >= SCALE_1 && scale < SCALE_ATREG) - { - SIB = true; - } - - if (scale == SCALE_ATREG && ((_offsetOrBaseReg & 7) == 4)) - { - SIB = true; - ireg = _offsetOrBaseReg; - } - - // Okay, we're fine. Just disp encoding. - // We need displacement. Which size? - int ioff = (int)(s64)offset; - if (ioff == 0 && (_offsetOrBaseReg & 7) != 5) - { - mod = 0; - } - else if (ioff < -128 || ioff > 127) - { - mod = 2; // 32-bit displacement - } - else - { - mod = 1; // 8-bit displacement - } + mod = 2; // 32-bit displacement } } From f82c38e156ffce02242dfad4df8b36c4e59b0432 Mon Sep 17 00:00:00 2001 From: Sintendo Date: Mon, 13 Jan 2020 00:13:05 +0100 Subject: [PATCH 4/5] X64Emitter: Remove obsolete TODO TODO was already taken care of in PR #941. --- Source/Core/Common/x64Emitter.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Source/Core/Common/x64Emitter.cpp b/Source/Core/Common/x64Emitter.cpp index a90af4c288..8878f07f65 100644 --- a/Source/Core/Common/x64Emitter.cpp +++ b/Source/Core/Common/x64Emitter.cpp @@ -13,7 +13,6 @@ namespace Gen { -// TODO(ector): Add EAX special casing, for ever so slightly smaller code. struct NormalOpDef { u8 toRm8, toRm32, fromRm8, fromRm32, imm8, imm32, simm8, eaximm8, eaximm32, ext; From bdcdd763fe37c8f78860987812f354b0c5c18232 Mon Sep 17 00:00:00 2001 From: Sintendo Date: Mon, 13 Jan 2020 00:15:10 +0100 Subject: [PATCH 5/5] x64Emitter: Remove unused macros No users, and one them seems to do the same as stddef.h's offsetof() already used elsewhere. --- Source/Core/Common/x64Emitter.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Source/Core/Common/x64Emitter.h b/Source/Core/Common/x64Emitter.h index 314f263012..45c1e15f1d 100644 --- a/Source/Core/Common/x64Emitter.h +++ b/Source/Core/Common/x64Emitter.h @@ -313,11 +313,6 @@ inline u32 PtrOffset(const void* ptr, const void* base = nullptr) return (u32)distance; } -// usage: int a[]; ARRAY_OFFSET(a,10) -#define ARRAY_OFFSET(array, index) ((u32)((u64) & (array)[index] - (u64) & (array)[0])) -// usage: struct {int e;} s; STRUCT_OFFSET(s,e) -#define STRUCT_OFFSET(str, elem) ((u32)((u64) & (str).elem - (u64) & (str))) - struct FixupBranch { enum class Type