Skip to content

[RISCV] Convert an assertion to an if condition in getRegAllocationHints #85998

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

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Mar 20, 2024

With GPR pairs from Zdinx, we can't guarantee there are no subregisters on integer instruction operands. I've been able to get these assertions to fire after some other recent PRs.

I've added a FIXME to support this properly. I just wanted to prevent the assertion failure for now.

No test case because my other patch that allowed me to fail the assert hasn't been approved yet, and I don't know for that that patch is required to hit this assert. It's just want exposed it for me. So I think this patch is a good precaution regardless.

With GPR pairs from Zdinx, we can't guarantee there are no subregisters on
integer instruction operands. I've been able to get these assertions
to fire after some other recent PRs.

I've added a FIXME to support this properly. I just wanted to prevent
the assertion failure for now.
@llvmbot
Copy link
Member

llvmbot commented Mar 20, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

With GPR pairs from Zdinx, we can't guarantee there are no subregisters on integer instruction operands. I've been able to get these assertions to fire after some other recent PRs.

I've added a FIXME to support this properly. I just wanted to prevent the assertion failure for now.

No test case because my other patch that allowed me to fail the assert hasn't been approved yet, and I don't know for that that patch is required to hit this assert. It's just want exposed it for me. So I think this patch is a good precaution regardless.


Full diff: https://github.com/llvm/llvm-project/pull/85998.diff

1 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp (+5-2)
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
index 10bf1e88d74146..952d17468da590 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
@@ -741,8 +741,11 @@ bool RISCVRegisterInfo::getRegAllocationHints(
                         bool NeedGPRC) -> void {
     Register Reg = MO.getReg();
     Register PhysReg = Reg.isPhysical() ? Reg : Register(VRM->getPhys(Reg));
-    if (PhysReg && (!NeedGPRC || RISCV::GPRCRegClass.contains(PhysReg))) {
-      assert(!MO.getSubReg() && !VRRegMO.getSubReg() && "Unexpected subreg!");
+    // TODO: Support GPRPair subregisters? Need to be careful with even/odd
+    // registers. If the virtual register is an odd register of a pair and the
+    // physical register is even (or vice versa), we should not add the hint.
+    if (PhysReg && (!NeedGPRC || RISCV::GPRCRegClass.contains(PhysReg)) &&
+        !MO.getSubReg() && !VRRegMO.getSubReg()) {
       if (!MRI->isReserved(PhysReg) && !is_contained(Hints, PhysReg))
         TwoAddrHints.insert(PhysReg);
     }

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@topperc topperc merged commit ce8e869 into llvm:main Mar 21, 2024
@topperc topperc deleted the pr/regalloc-asserts branch March 21, 2024 05:53
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
…nts (llvm#85998)

With GPR pairs from Zdinx, we can't guarantee there are no subregisters
on integer instruction operands. I've been able to get these assertions
to fire after some other recent PRs.

I've added a FIXME to support this properly. I just wanted to prevent
the assertion failure for now.

No test case because my other patch llvm#85982 that allowed me to fail the assert
hasn't been approved yet, and I don't know for that that patch is
required to hit this assert. It's just what exposed it for me. So I
think this patch is a good precaution regardless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants