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.

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

Arm64Gen: Convert ARM64Reg to enum class #9494

Merged

Conversation

Dentomologist
Copy link
Contributor

@Dentomologist Dentomologist commented Feb 6, 2021

Most changes are just adding ARM64Reg:: in front of the constants.

Add operator functions to reduce type casting spam

Make DecodeReg return int instead of ARM64Reg

Add EncodeRegTo32 with previous behavior of DecodeReg

Despite appearances, this does not change the value of
ARM64Reg::INVALID_REG. The previous value 0xFFFFFFFF was being cast to
an int and wrapping around to -1, this just makes that value explicit.

@Dentomologist Dentomologist marked this pull request as draft February 6, 2021 19:04
@Dentomologist Dentomologist force-pushed the convert_arm64reg_to_enum_class branch 2 times, most recently from e458ded to 6f38e16 Compare February 9, 2021 01:41
@Dentomologist Dentomologist force-pushed the convert_arm64reg_to_enum_class branch 4 times, most recently from 8f5dc50 to 25cc742 Compare February 12, 2021 18:08
INVALID_REG = 0xFFFFFFFF
INVALID_REG = -1,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually change the value of INVALID_REG; 0xFFFFFFFF was overflowing the int underlying type and wrapping around to -1.

@Dentomologist Dentomologist force-pushed the convert_arm64reg_to_enum_class branch 2 times, most recently from eaccf53 to 55a529a Compare March 7, 2021 20:53
@@ -1223,44 +1177,44 @@ void ARM64XEmitter::_MSR(PStateField field, ARM64Reg Rt)
int o0 = 0, op1 = 0, CRn = 0, CRm = 0, op2 = 0;
ASSERT_MSG(DYNA_REC, Is64Bit(Rt), "MSR: Rt must be 64-bit");
GetSystemReg(field, o0, op1, CRn, CRm, op2);
EncodeSystemInst(o0, op1, CRn, CRm, op2, DecodeReg(Rt));
EncodeSystemInst(o0, op1, CRn, CRm, op2, Rt);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rt is decoded inside EncodeSystemInst, so there's no need to do it here.

}
constexpr ARM64Reg EncodeRegTo64(ARM64Reg reg)
{
return static_cast<ARM64Reg>(reg | 0x20);
}
constexpr ARM64Reg EncodeRegToSingle(ARM64Reg reg)
{
return static_cast<ARM64Reg>(DecodeReg(reg) + S0);
return static_cast<ARM64Reg>(ARM64Reg::S0 | DecodeReg(reg));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S0 = 64 and DecodeReg return value is in range [0, 31] so these are equivalent.

@Dentomologist
Copy link
Contributor Author

As per previous discussion:

Made DecodeReg return int, and added EncodeRegTo32 which returns ARM64Reg
Replaced DecodeReg with EncodeRegTo32 where appropriate
Removed GetUsedRegisterIndex function and replaced calls with DecodeReg
Removed extraneous operator functions

@Dentomologist
Copy link
Contributor Author

One last thing before I remove draft status: this builds properly and isn't supposed to change any functionality, but since I don't have an ARM device to test it myself it would be helpful for someone who does to run a few games as a sanity check.

@JosJuice
Copy link
Member

I did some quick tests of a few games. Things seem to be working fine.

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM, but you should probably remove the lines of the commit message that come after "Most changes are just adding ARM64Reg:: in front of the constants."

Most changes are just adding ARM64Reg:: in front of the constants.
@Dentomologist Dentomologist marked this pull request as ready for review March 13, 2021 18:30
@Dentomologist
Copy link
Contributor Author

Code changes LGTM, but you should probably remove the lines of the commit message that come after "Most changes are just adding ARM64Reg:: in front of the constants."

Done, and draft status removed.

@JosJuice
Copy link
Member

Gonna take the opportunity to merge this before someone merges one of my open JitArm64 PRs and causes a merge conflict. degasus was the one who suggested making this into an enum class so I'm going to assume he doesn't mind this being merged

@JosJuice JosJuice merged commit a45a0a2 into dolphin-emu:master Mar 16, 2021
10 checks passed
@Dentomologist Dentomologist deleted the convert_arm64reg_to_enum_class branch March 19, 2021 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants