Clean up and improve documentation of x86_64 registers #1630
Conversation
hainest
added
code cleanup
|
Testing fine on all platforms. |
There was a problem hiding this comment.
A couple changes in getBaseRegister, then it looks good to me.
If you feel ambitious, you could make and use mask constants and for the components of a register (id, alias/subrange, category, arch), so it is clearer, and less implementation details are spilled everywhere. Even more ambitious would be to create and use functions to get the components of the register that use the masks.
common/h/registers/x86_64_regs.h
Outdated
| const signed int YMM = 0x000B0000; // YMM0-YMM7 Registers from AVX2/FMA | ||
| const signed int ZMM = 0x000C0000; // ZMM0-ZMM7 Registers from AVX-512 |
There was a problem hiding this comment.
update comment, there are 16 YMM and 32 ZMM registers.
common/h/registers/x86_64_regs.h
Outdated
| const signed int XMM = 0x000A0000; // XMM0-XMM7 Registers from SSE | ||
| const signed int YMM = 0x000B0000; // YMM0-YMM7 Registers from AVX2/FMA | ||
| const signed int ZMM = 0x000C0000; // ZMM0-ZMM7 Registers from AVX-512 |
There was a problem hiding this comment.
update comment, in x86_64 there are 16 YMM and 32 ZMM registers, but should probably say 32 XMM, YMM and ZMM registers as that is what AVX-512 can access.
There was a problem hiding this comment.
I added notes for the extension that added the registers and the values for AVX-512.
common/src/registers/MachRegister.C
Outdated
| return MachRegister(reg & 0xfffff0ff); | ||
| else if(category == x86_64::FLAG) | ||
| return x86_64::flags; | ||
| else if(category == x86_64::MMX) | ||
| return x86_64::st0; | ||
| else if(category == x86_64::XMM) | ||
| return x86_64::ymm0; | ||
| else if(category == x86_64::YMM) | ||
| return x86_64::zmm0; | ||
| else | ||
| return *this; |
There was a problem hiding this comment.
XMM should return ZMM as it is the largest containing register.
A switch statement would be a better and match the rest of the code.
The return value for MMX, XMM, and YMM should change the category and leave the register number so the base register of xmm5 is zmm5.
There was a problem hiding this comment.
XMM should return ZMM as it is the largest containing register.
Fixed.
A switch statement would be a better and match the rest of the code.
Agreed, but I'll put that off until another PR.
The return value for MMX, XMM, and YMM should change the category and leave the register number so the base register of xmm5 is zmm5.
That makes sense. Updated.
common/src/registers/MachRegister.C
Outdated
| @@ -39,6 +39,12 @@ namespace Dyninst { | |||
| return MachRegister(reg & 0xfffff0ff); | |||
There was a problem hiding this comment.
clear the whole 16-bits of the subrange with a mask of 0xffff00ff.
I think a small, but useful, first step is to make member functions to get each piece (like regClass() that's never used...). Ultimately, I think putting all of these values into enums and giving MachRegister a real constructor that can deal with types for the fields instead of bits in a |
The MMX* registers are only the lower 64 bits of the st* ones.
The values in the two namespaces are not the same.
This puts them in more of a chronological order.
This puts them in more of a chronological order.
No real comments here. Just added a separator.
FLAGC is the lower bit of the I/O Permission Level field.
FLAGC is the upper bit of the I/O Permission Level field.
It's 64 bits, not 128. From Intel(R) 64 and IA-32 Architectures Software Developer’s Manual June 2021 15.6.1 OPMASK Register to Predicate Vector Data Processing The opmask is a set of eight architectural registers of size MAX_KL (64-bit).
This makes it consistent with the names used for the other vector extensions.
It makes it easier to read.
Assume AMD64 extensions are in place so that XMM8-XMM15 are present.
hainest
force-pushed
the
thaines/x86_64_registers
branch
from
de91da7
to
18d78f4
Compare
|
Looks good. |
The values for the RFLAGS fields VM..ID will get updated to the x86 ones after #1629 is merged.