[RISCV][MRI] Account for fixed registers when determining callee saved regs #115756

michaelmaitland · 2024-11-11T18:48:19Z

This fixes https://discourse.llvm.org/t/fixed-register-being-spill-and-restored-in-clang/83058.

We need to do it in MachineRegisterInfo::getCalleeSavedRegs instead of RISCVRegisterInfo::getCalleeSavedRegs since the MF argument of TargetRegisterInfo:::getCalleeSavedRegs is const, so we can't call MF->getRegInfo().disableCalleeSavedRegister there.

So to put it in MachineRegisterInfo::getCalleeSavedRegs, we move isRegisterReservedByUser into TargetSubtargetInfo.

…d regs

llvmbot · 2024-11-11T18:48:41Z

@llvm/pr-subscribers-backend-m68k

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

Author: Michael Maitland (michaelmaitland)

Changes

This fixes https://discourse.llvm.org/t/fixed-register-being-spill-and-restored-in-clang/83058.

We need to do it in MachineRegisterInfo::getCalleeSavedRegs instead of RISCVRegisterInfo::getCalleeSavedRegs since the MF argument of TargetRegisterInfo:::getCalleeSavedRegs is const, so we can't call MF->getRegInfo().disableCalleeSavedRegister there.

So to put it in MachineRegisterInfo::getCalleeSavedRegs, we move isRegisterReservedByUser into TargetSubtargetInfo.

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

8 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetSubtargetInfo.h (+2)
(modified) llvm/lib/CodeGen/MachineRegisterInfo.cpp (+7-1)
(modified) llvm/lib/Target/M68k/M68kRegisterInfo.cpp (+1-1)
(modified) llvm/lib/Target/M68k/M68kSubtarget.h (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+1-2)
(modified) llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVSubtarget.h (+1-1)
(added) llvm/test/CodeGen/RISCV/fixed-global-regalloc.ll (+20)

diff --git a/llvm/include/llvm/CodeGen/TargetSubtargetInfo.h b/llvm/include/llvm/CodeGen/TargetSubtargetInfo.h
index bfaa6450779ae0..6b14d1476568f9 100644
--- a/llvm/include/llvm/CodeGen/TargetSubtargetInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetSubtargetInfo.h
@@ -339,6 +339,8 @@ class TargetSubtargetInfo : public MCSubtargetInfo {
     // Conservatively assume such instructions exist by default.
     return true;
   }
+
+  virtual bool isRegisterReservedByUser(Register R) const { return false; }
 };
 } // end namespace llvm
 
diff --git a/llvm/lib/CodeGen/MachineRegisterInfo.cpp b/llvm/lib/CodeGen/MachineRegisterInfo.cpp
index fcedb302d228c4..6f636a161f5000 100644
--- a/llvm/lib/CodeGen/MachineRegisterInfo.cpp
+++ b/llvm/lib/CodeGen/MachineRegisterInfo.cpp
@@ -635,7 +635,13 @@ const MCPhysReg *MachineRegisterInfo::getCalleeSavedRegs() const {
   if (IsUpdatedCSRsInitialized)
     return UpdatedCSRs.data();
 
-  return getTargetRegisterInfo()->getCalleeSavedRegs(MF);
+  const MCPhysReg *Regs = getTargetRegisterInfo()->getCalleeSavedRegs(MF);
+
+  for (unsigned I = 0; Regs[I]; ++I)
+    if (MF->getSubtarget().isRegisterReservedByUser(Regs[I]))
+      MF->getRegInfo().disableCalleeSavedRegister(Regs[I]);
+
+  return Regs;
 }
 
 void MachineRegisterInfo::setCalleeSavedRegs(ArrayRef<MCPhysReg> CSRs) {
diff --git a/llvm/lib/Target/M68k/M68kRegisterInfo.cpp b/llvm/lib/Target/M68k/M68kRegisterInfo.cpp
index 62fb72ba4fd5e4..0f6b4761f2cb8a 100644
--- a/llvm/lib/Target/M68k/M68kRegisterInfo.cpp
+++ b/llvm/lib/Target/M68k/M68kRegisterInfo.cpp
@@ -136,7 +136,7 @@ BitVector M68kRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
 
   // Registers reserved by users
   for (size_t Reg = 0, Total = getNumRegs(); Reg != Total; ++Reg) {
-    if (MF.getSubtarget<M68kSubtarget>().isRegisterReservedByUser(Reg))
+    if (MF.getSubtarget().isRegisterReservedByUser(Reg))
       setBitVector(Reg);
   }
 
diff --git a/llvm/lib/Target/M68k/M68kSubtarget.h b/llvm/lib/Target/M68k/M68kSubtarget.h
index 3fbec2f72fb861..c08a9786fb27ba 100644
--- a/llvm/lib/Target/M68k/M68kSubtarget.h
+++ b/llvm/lib/Target/M68k/M68kSubtarget.h
@@ -107,7 +107,7 @@ class M68kSubtarget : public M68kGenSubtargetInfo {
 
   bool isPositionIndependent() const;
 
-  bool isRegisterReservedByUser(Register R) const {
+  bool isRegisterReservedByUser(Register R) const override {
     assert(R < M68k::NUM_TARGET_REGS && "Register out of range");
     return UserReservedRegister[R];
   }
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 831b0b30d47fcc..c8c495327c383f 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -19884,8 +19884,7 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
   // reserved, if so report an error. Do the same for the return address if this
   // is not a tailcall.
   validateCCReservedRegs(RegsToPass, MF);
-  if (!IsTailCall &&
-      MF.getSubtarget<RISCVSubtarget>().isRegisterReservedByUser(RISCV::X1))
+  if (!IsTailCall && MF.getSubtarget().isRegisterReservedByUser(RISCV::X1))
     MF.getFunction().getContext().diagnose(DiagnosticInfoUnsupported{
         MF.getFunction(),
         "Return address register required, but has been reserved."});
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
index 26195ef721db39..4b3b1d5154e9f6 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
@@ -163,7 +163,7 @@ BitVector RISCVRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
 
 bool RISCVRegisterInfo::isAsmClobberable(const MachineFunction &MF,
                                          MCRegister PhysReg) const {
-  return !MF.getSubtarget<RISCVSubtarget>().isRegisterReservedByUser(PhysReg);
+  return !MF.getSubtarget().isRegisterReservedByUser(PhysReg);
 }
 
 const uint32_t *RISCVRegisterInfo::getNoPreservedMask() const {
diff --git a/llvm/lib/Target/RISCV/RISCVSubtarget.h b/llvm/lib/Target/RISCV/RISCVSubtarget.h
index f59a3737ae76f9..99edf22eb0791f 100644
--- a/llvm/lib/Target/RISCV/RISCVSubtarget.h
+++ b/llvm/lib/Target/RISCV/RISCVSubtarget.h
@@ -218,7 +218,7 @@ class RISCVSubtarget : public RISCVGenSubtargetInfo {
            TargetABI == RISCVABI::ABI_ILP32 ||
            TargetABI == RISCVABI::ABI_ILP32E;
   }
-  bool isRegisterReservedByUser(Register i) const {
+  bool isRegisterReservedByUser(Register i) const override {
     assert(i < RISCV::NUM_TARGET_REGS && "Register out of range");
     return UserReservedRegister[i];
   }
diff --git a/llvm/test/CodeGen/RISCV/fixed-global-regalloc.ll b/llvm/test/CodeGen/RISCV/fixed-global-regalloc.ll
new file mode 100644
index 00000000000000..f1993ae142ae8f
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/fixed-global-regalloc.ll
@@ -0,0 +1,20 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc %s -mtriple=riscv64 -mattr=+reserve-x24 | FileCheck %s
+
+define i32 @main() {
+; CHECK-LABEL: main:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    li s8, 123
+; CHECK-NEXT:    li a0, 0
+; CHECK-NEXT:    ret
+entry:
+  tail call void @llvm.write_register.i64(metadata !0, i64 123)
+  ret i32 0
+}
+
+declare void @llvm.write_register.i64(metadata, i64) #1
+
+!llvm.named.register.x24 = !{!0}
+
+!0 = !{!"x24"}
+

topperc · 2024-11-11T18:54:51Z

Does this change the stack layout of callee saved registers when one is reserved? Does it match gcc stack layout?

michaelmaitland · 2024-11-11T19:23:25Z

Does this change the stack layout of callee saved registers when one is reserved?

This patch causes us not to spill the reserved register, when we were spilling it before. This by definition changes the stack layout.

Does it match gcc stack layout?

I tried with this program with my patch applied:

register long a asm("x24");
int bar();
int baz(int a, int b, int c, int d);
int foo(int b, int c, int d) {
  a = 123;
  bar();
  baz(a, b, c, d);
  return 0;
}

GCC:

sd      ra,24(sp)
sd      s0,16(sp)
sd      s1,8(sp)
sd      s2,0(sp)

LLVM + this patch:

sd ra, 24(sp) # 8-byte Folded Spill
sd s0, 16(sp) # 8-byte Folded Spill
sd s1, 8(sp) # 8-byte Folded Spill
sd s2, 0(sp) # 8-byte Folded Spill

Looks the same on this example.

topperc · 2024-11-11T19:43:11Z

Does this change the stack layout of callee saved registers when one is reserved?

This patch causes us not to spill the reserved register, when we were spilling it before. This by definition changes the stack layout.

Does it match gcc stack layout?

I tried with this program with my patch applied:
register long a asm("x24");
int bar();
int baz(int a, int b, int c, int d);
int foo(int b, int c, int d) {
  a = 123;
  bar();
  baz(a, b, c, d);
  return 0;
}
GCC:
sd      ra,24(sp)
sd      s0,16(sp)
sd      s1,8(sp)
sd      s2,0(sp)
LLVM + this patch:
sd ra, 24(sp) # 8-byte Folded Spill
sd s0, 16(sp) # 8-byte Folded Spill
sd s1, 8(sp) # 8-byte Folded Spill
sd s2, 0(sp) # 8-byte Folded Spill
Looks the same on this example.

A better test would spill s7 and s9, but not s8. I want to know if s8 still gets allocated a slot in gcc.

michaelmaitland · 2024-11-11T21:47:35Z

It looks like we are still allocating and spilling a slack slot. I think GCC is over allocating but not spilling?

register long a asm("x8");

int foo(int b, int c) {
  asm volatile ("" ::: "x9");
  a = 321;
  return 0;
}

GCC:

        addi    sp,sp,-16
        sd      s1,8(sp)
        li      s0,321
        ld      s1,8(sp)
        li      a0,0
        addi    sp,sp,16
        jr      ra

LLVM + this patch:

        addi    sp, sp, -16
        sd      s0, 8(sp)                       # 8-byte Folded Spill
        sd      s1, 0(sp)                       # 8-byte Folded Spill
        li      s0, 321
        li      a0, 0
        ld      s0, 8(sp)                       # 8-byte Folded Reload
        ld      s1, 0(sp)                       # 8-byte Folded Reload
        addi    sp, sp, 16
        ret

topperc · 2024-11-11T21:50:06Z

LLVM + this patch:

        addi    sp, sp, -16
        sd      s0, 8(sp)                       # 8-byte Folded Spill
        sd      s1, 0(sp)                       # 8-byte Folded Spill
        li      s0, 321
        li      a0, 0
        ld      s0, 8(sp)                       # 8-byte Folded Reload
        ld      s1, 0(sp)                       # 8-byte Folded Reload
        addi    sp, sp, 16
        ret

Shouldn't this patch prevent the save/restore of s0?

jrtc27 · 2024-11-11T21:50:22Z

Reserving x8 doesn't seem like a sensible thing to do, that's one of the few registers that really should be under the compiler's control, no?

michaelmaitland · 2024-11-11T22:02:50Z

LLVM + this patch:

        addi    sp, sp, -16
        sd      s0, 8(sp)                       # 8-byte Folded Spill
        sd      s1, 0(sp)                       # 8-byte Folded Spill
        li      s0, 321
        li      a0, 0
        ld      s0, 8(sp)                       # 8-byte Folded Reload
        ld      s1, 0(sp)                       # 8-byte Folded Reload
        addi    sp, sp, 16
        ret

Shouldn't this patch prevent the save/restore of s0?

Sorry, I collected this experiment without this patch (I had built another branch and forgot). I have updated the test cases now with the correct code. And I addressed @jrtc27 concern of using x8.

michaelmaitland · 2024-11-12T16:41:30Z

isRegisterReservedByUser has the same behavior on both RISC-V and M68k. I think it makes sense to use a common interface.
On LLVM, M68k target does not allow named registers in ASM. I've added a test to show this.
godbolt shows us that GCC does not save/restore for fixed registers. I think it makes sense for M68k target to eventually get the behavior in this patch.
It is out of the scope of this PR to add named register support to M68k target, and they will have to modify the test added in this patch when they go to add this support.

llvm/test/CodeGen/M68k/fixed-csr.ll

michaelmaitland · 2024-11-12T18:59:24Z

The behavior differs from GCC for AArch64, ARM, Hexagon, SPARC, M68k and RISC-V.

In LLVM, different targets have different interfaces to check whether the register was -ffixed-XXX. I am proposing that each target migrate to this common interface in follow up patches.

This patch adds the interface, and migrates RISC-V and M68k to that interface, since they are already using the interface.

topperc · 2024-11-12T19:08:46Z

The behavior differs from GCC for AArch64, ARM, Hexagon, SPARC, M68k and RISC-V.

In LLVM, different targets have different interfaces to check whether the register was -ffixed-XXX. I am proposing that each target migrate to this common interface in follow up patches.

This patch adds the interface, and migrates RISC-V and M68k to that interface, since they are already using the interface.

How will this interact with AArch64 or other targets calling setCalleeSavedRegisters which will cause IsUpdatedCSRsInitialized to get set preventing reaching the new code you added in getCalleeSavedRegs

michaelmaitland · 2024-11-14T18:23:45Z

The behavior differs from GCC for AArch64, ARM, Hexagon, SPARC, M68k and RISC-V.
In LLVM, different targets have different interfaces to check whether the register was -ffixed-XXX. I am proposing that each target migrate to this common interface in follow up patches.
This patch adds the interface, and migrates RISC-V and M68k to that interface, since they are already using the interface.

How will this interact with AArch64 or other targets calling setCalleeSavedRegisters which will cause IsUpdatedCSRsInitialized to get set preventing reaching the new code you added in getCalleeSavedRegs

If a target calls setCalleeSavedRegs, then its saying "give me full control of what registers are callee saved regs". If a target calls setCalleeSavedRegs, then it is on the target to determine which registers are fixed, in the same way its the target to figure out what registers are callee saved regs.

If a target calls getCalleeSavedRegs and IsUpdatedCSRsInitialized is true, it is prevented from reaching the existing getTargetRegisterInfo()->getCalleeSavedRegs() code. How is this any different?

topperc · 2024-11-14T18:44:21Z

The behavior differs from GCC for AArch64, ARM, Hexagon, SPARC, M68k and RISC-V.
In LLVM, different targets have different interfaces to check whether the register was -ffixed-XXX. I am proposing that each target migrate to this common interface in follow up patches.
This patch adds the interface, and migrates RISC-V and M68k to that interface, since they are already using the interface.

How will this interact with AArch64 or other targets calling setCalleeSavedRegisters which will cause IsUpdatedCSRsInitialized to get set preventing reaching the new code you added in getCalleeSavedRegs

If a target calls setCalleeSavedRegs, then its saying "give me full control of what registers are callee saved regs". If a target calls setCalleeSavedRegs, then it is on the target to determine which registers are fixed, in the same way its the target to figure out what registers are callee saved regs.

If a target calls getCalleeSavedRegs and IsUpdatedCSRsInitialized is true, it is prevented from reaching the existing getTargetRegisterInfo()->getCalleeSavedRegs() code. How is this any different?

The existing AArch64 code that uses setCalleeSavedRegs calls AArch64RegisterInfo::getCalleeSavedRegs and then adds some stuff to it. I guess the code you added to getCalleeSavedRegs will need to be copied into AArch64RegisterInfo::UpdateCustomCalleeSavedRegs to remove the fixed registers?

I'm just asking questions to make sure you've designed something that covers the use cases of other targets. If you think having AArch64 fix implement their fixed register support in AArch64RegisterInfo::UpdateCustomCalleeSavedRegs is the best solution, I'm ok with that.

michaelmaitland · 2024-11-14T19:08:59Z

The existing AArch64 code that uses setCalleeSavedRegs calls AArch64RegisterInfo::getCalleeSavedRegs and then adds some stuff to it. I guess the code you added to getCalleeSavedRegs will need to be copied into AArch64RegisterInfo::UpdateCustomCalleeSavedRegs to remove the fixed registers?

I'm interpreting the existing code in AArch64 as copying the call of getTargetRegisterInfo->getCalleeSavedRegs out of MachineRegisterInfo::getCalleeSavedRegs and putting it into AArch64RegisterInfo::UpdateCustomCalleeSavedRegs. So yes, they would also need to copy the code to copy the code to remove the fixed registers.

Or they could just call MachineRegisterInfo::getCalleeSavedRegs from the start, add some regs to the result, and call setCalleeSavedRegs. Or they could use getCalleeSavedRegs and add a function addCalleeSavedReg instead of using setCalleeSavedRegs.

michaelmaitland · 2024-12-02T18:21:51Z

ping

arichardson · 2024-12-02T21:55:09Z

llvm/test/CodeGen/RISCV/fixed-csr.ll

+
+declare void @llvm.write_register.i64(metadata, i64)
+
+define noundef signext i32 @bar() {


could add nounwind here to remove the .cfi directives from the test.

topperc

LGTM

michaelmaitland added 2 commits November 11, 2024 07:32

[RISCV][RegAlloc] Pre-commit test case

55641bb

[MRI][RISCV] Account for fixed registers when determining callee save…

3c661fe

…d regs

michaelmaitland added the backend:RISC-V label Nov 11, 2024

michaelmaitland requested review from asb, preames and topperc November 11, 2024 18:48

llvmbot added the backend:m68k label Nov 11, 2024

fixup! fix test checks RUN string

958aeb3

fixup spill next register

dc3a58b

michaelmaitland requested a review from mshockwave November 12, 2024 16:36

fixup! add m68k test

5af3161

topperc reviewed Nov 12, 2024

View reviewed changes

llvm/test/CodeGen/M68k/fixed-csr.ll Outdated Show resolved Hide resolved

fixup! use callee saved register

59dc0b7

fixup! fremove M68k from patch

857380f

michaelmaitland requested a review from topperc December 2, 2024 18:21

arichardson reviewed Dec 2, 2024

View reviewed changes

michaelmaitland requested a review from arichardson December 5, 2024 14:50

fixup! add nounwind to test

b6f1191

topperc approved these changes Dec 5, 2024

View reviewed changes

michaelmaitland merged commit 84efad0 into llvm:main Dec 6, 2024
8 checks passed

MaskRay added a commit that referenced this pull request Dec 7, 2024

[M68k] Fix -Winconsistent-missing-override after #115756

9d64af9


		declare void @llvm.write_register.i64(metadata, i64)

		define noundef signext i32 @bar() {

[RISCV][MRI] Account for fixed registers when determining callee saved regs #115756

[RISCV][MRI] Account for fixed registers when determining callee saved regs #115756

Uh oh!

Conversation

michaelmaitland commented Nov 11, 2024

Uh oh!

llvmbot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Nov 11, 2024

Uh oh!

michaelmaitland commented Nov 11, 2024

Uh oh!

topperc commented Nov 11, 2024

Uh oh!

michaelmaitland commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Nov 11, 2024

Uh oh!

jrtc27 commented Nov 11, 2024

Uh oh!

michaelmaitland commented Nov 11, 2024

Uh oh!

michaelmaitland commented Nov 12, 2024

Uh oh!

Uh oh!

michaelmaitland commented Nov 12, 2024

Uh oh!

topperc commented Nov 12, 2024

Uh oh!

michaelmaitland commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Nov 14, 2024

Uh oh!

michaelmaitland commented Nov 14, 2024

Uh oh!

michaelmaitland commented Dec 2, 2024

Uh oh!

arichardson Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

michaelmaitland Dec 5, 2024

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Nov 11, 2024 •

edited

Loading

michaelmaitland commented Nov 11, 2024 •

edited

Loading

michaelmaitland commented Nov 14, 2024 •

edited

Loading