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

JitArm64: Fix crandc/creqv/crorc setting eq bit #13266

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

JitArm64: Fix crandc/creqv/crorc setting eq bit #13266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
411ebad
JitArm64: Fix crandc/creqv/crorc setting eq bit
JosJuice Jan 6, 2025
6678af3
JitArm64: Optimize creqv setting eq/gt bit
JosJuice Jan 6, 2025
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
5 changes: 4 additions & 1 deletion Source/Core/Core/PowerPC/JitArm64/Jit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,10 @@ class JitArm64 : public JitBase, public Arm64Gen::ARM64CodeBlock, public CommonA
void WriteBLRExit(Arm64Gen::ARM64Reg dest);

void GetCRFieldBit(int field, int bit, Arm64Gen::ARM64Reg out);
void SetCRFieldBit(int field, int bit, Arm64Gen::ARM64Reg in, bool negate = false);
// This assumes that all bits except for bit 0 (LSB) are set to 0. But if bits_1_to_31_are_set
// equals true, it instead assumes that all of bits 1 to 31 are set.
void SetCRFieldBit(int field, int bit, Arm64Gen::ARM64Reg in, bool negate = false,
bool bits_1_to_31_are_set = false);
void ClearCRFieldBit(int field, int bit);
void SetCRFieldBit(int field, int bit);
void FixGTBeforeSettingCRFieldBit(Arm64Gen::ARM64Reg reg);
Expand Down
35 changes: 27 additions & 8 deletions Source/Core/Core/PowerPC/JitArm64/JitArm64_SystemRegisters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ void JitArm64::GetCRFieldBit(int field, int bit, ARM64Reg out)
}
}

void JitArm64::SetCRFieldBit(int field, int bit, ARM64Reg in, bool negate)
void JitArm64::SetCRFieldBit(int field, int bit, ARM64Reg in, bool negate,
bool bits_1_to_31_are_set)
{
gpr.BindCRToRegister(field, true);
ARM64Reg CR = gpr.CR(field);
Expand All @@ -70,7 +71,9 @@ void JitArm64::SetCRFieldBit(int field, int bit, ARM64Reg in, bool negate)
AND(CR, CR, LogicalImm(0xFFFF'FFFF'0000'0000, GPRSize::B64));
ORR(CR, CR, in);
if (!negate)
EOR(CR, CR, LogicalImm(1ULL << 0, GPRSize::B64));
EOR(CR, CR, LogicalImm(bits_1_to_31_are_set ? 0xFFFF'FFFFULL : 1ULL, GPRSize::B64));
else if (bits_1_to_31_are_set)
AND(CR, CR, LogicalImm(0xFFFF'FFFF'0000'0001ULL, GPRSize::B64));
break;
Comment on lines 71 to 77
Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose we replaced this whole CR_EQ_BIT case with a BFI of the lower 32 bits (which would handle the combination where negate is true and bits_1_to_31_are_set is false), then EOR with 0x1, 0xFFFF'FFFE, or 0xFFFF'FFFF for the other three combinations respectively. That would make each combination 1 or 2 instructions; are there any downsides I'm missing?


case PowerPC::CR_GT_BIT: // set bit 63 to !input
Expand Down Expand Up @@ -632,8 +635,12 @@ void JitArm64::crXXX(UGeckoInstruction inst)
}
}

// crnor or crnand
const bool negate_result = inst.SUBOP10 == 33 || inst.SUBOP10 == 225;
const u32 crbd_bit = 3 - (inst.CRBD & 3);
// crnor, crnand and sometimes creqv
const bool negate_result =
inst.SUBOP10 == 33 || inst.SUBOP10 == 225 ||
(inst.SUBOP10 == 289 && (crbd_bit == PowerPC::CR_EQ_BIT || crbd_bit == PowerPC::CR_GT_BIT));
bool bits_1_to_31_are_set = false;

auto WA = gpr.GetScopedReg();
ARM64Reg XA = EncodeRegTo64(WA);
Expand All @@ -653,15 +660,26 @@ void JitArm64::crXXX(UGeckoInstruction inst)
break;

case 129: // crandc: A && ~B
BIC(XA, XA, XB);
BIC(WA, WA, WB);
bits_1_to_31_are_set = true;
break;
Comment on lines 662 to 665
Copy link
Contributor

Choose a reason for hiding this comment

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

A is either 0 or 1, right? If so A && ~B should result in 0 or 1 too.


case 193: // crxor: A ^ B
EOR(XA, XA, XB);
break;

case 289: // creqv: ~(A ^ B) = A ^ ~B
EON(XA, XA, XB);
// Both of these two implementations are equally correct, but which one is more efficient
// depends on which bit we're going to set in CRBD
if (negate_result)
{
ORR(XA, XA, XB);
}
else
{
EON(WA, WA, WB);
bits_1_to_31_are_set = true;
}
break;

case 33: // crnor: ~(A || B)
Expand All @@ -670,13 +688,14 @@ void JitArm64::crXXX(UGeckoInstruction inst)
break;

case 417: // crorc: A || ~B
ORN(XA, XA, XB);
ORN(WA, WA, WB);
bits_1_to_31_are_set = true;
break;
}
}

// Store result bit in CRBD
SetCRFieldBit(inst.CRBD >> 2, 3 - (inst.CRBD & 3), XA, negate_result);
SetCRFieldBit(inst.CRBD >> 2, 3 - (inst.CRBD & 3), XA, negate_result, bits_1_to_31_are_set);
}

void JitArm64::mfcr(UGeckoInstruction inst)
Expand Down