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

[PowerPC] Expand global named register support #112603

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

lei137
Copy link
Contributor

@lei137 lei137 commented Oct 16, 2024

Enable all valid registers for intrinsics that read from and write
to global named registers.

@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-backend-powerpc

Author: Lei Huang (lei137)

Changes

Enable all valid registers for intrinsics that read from and write
to global named registers.


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

4 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCISelLowering.cpp (+17-13)
  • (modified) llvm/test/CodeGen/PowerPC/named-reg-alloc-r0.ll (+4-5)
  • (modified) llvm/test/CodeGen/PowerPC/named-reg-alloc-r2-64.ll (+1-2)
  • (modified) llvm/test/CodeGen/PowerPC/named-reg-alloc-r2.ll (+1-2)
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index cec1e507f08f2f..98b0a5ad4db0ee 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -17319,25 +17319,29 @@ SDValue PPCTargetLowering::LowerFRAMEADDR(SDValue Op,
   return FrameAddr;
 }
 
-// FIXME? Maybe this could be a TableGen attribute on some registers and
-// this table could be generated automatically from RegInfo.
+#define GET_REGISTER_MATCHER
+#include "PPCGenAsmMatcher.inc"
+
 Register PPCTargetLowering::getRegisterByName(const char* RegName, LLT VT,
                                               const MachineFunction &MF) const {
-  bool isPPC64 = Subtarget.isPPC64();
 
-  bool is64Bit = isPPC64 && VT == LLT::scalar(64);
-  if (!is64Bit && VT != LLT::scalar(32))
+  bool Is64Bit = Subtarget.isPPC64() && VT == LLT::scalar(64);
+  if (!Is64Bit && VT != LLT::scalar(32))
     report_fatal_error("Invalid register global variable type");
 
-  Register Reg = StringSwitch<Register>(RegName)
-                     .Case("r1", is64Bit ? PPC::X1 : PPC::R1)
-                     .Case("r2", isPPC64 ? Register() : PPC::R2)
-                     .Case("r13", (is64Bit ? PPC::X13 : PPC::R13))
-                     .Default(Register());
+  Register Reg = MatchRegisterName(RegName);
+  if (!Reg)
+    report_fatal_error(Twine("Invalid global name register \""
+                              + StringRef(RegName)  + "\"."));
+
+  // Convert GPR to GP8R register for 64bit.
+  if (Is64Bit && StringRef(RegName).starts_with_insensitive("r"))
+    Reg = Reg.id() - PPC::R0 + PPC::X0;
 
-  if (Reg)
-    return Reg;
-  report_fatal_error("Invalid register name global variable");
+  if (!Subtarget.getRegisterInfo()->getReservedRegs(MF).test(Reg))
+    report_fatal_error(Twine("Trying to obtain non-reservable register \"" +
+                             StringRef(RegName) + "\"."));
+  return Reg;
 }
 
 bool PPCTargetLowering::isAccessedAsGotIndirect(SDValue GA) const {
diff --git a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r0.ll b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r0.ll
index 11cb72296e2c43..be2fae8f3458be 100644
--- a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r0.ll
+++ b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r0.ll
@@ -1,11 +1,10 @@
-; RUN: not --crash llc < %s -mtriple=powerpc-unknown-linux-gnu 2>&1 | FileCheck %s
-; RUN: not --crash llc < %s -mtriple=powerpc-unknown-linux-gnu 2>&1 | FileCheck %s
-; RUN: not --crash llc < %s -mtriple=powerpc64-unknown-linux-gnu 2>&1 | FileCheck %s
+; RUN: not --crash llc -O0 < %s -mtriple=powerpc-unknown-linux-gnu 2>&1 | FileCheck %s
+; RUN: not --crash llc -O0 < %s -mtriple=powerpc-unknown-linux-gnu 2>&1 | FileCheck %s
+; RUN: not --crash llc -O0 < %s -mtriple=powerpc64-unknown-linux-gnu 2>&1 | FileCheck %s
 
 define i32 @get_reg() nounwind {
 entry:
-; FIXME: Include an allocatable-specific error message
-; CHECK: Invalid register name global variable
+; CHECK: Trying to obtain non-reservable register "r0".
         %reg = call i32 @llvm.read_register.i32(metadata !0)
   ret i32 %reg
 }
diff --git a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2-64.ll b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2-64.ll
index 3df778f445c733..b245c7d6f76c12 100644
--- a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2-64.ll
+++ b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2-64.ll
@@ -3,8 +3,7 @@
 
 define i64 @get_reg() nounwind {
 entry:
-; FIXME: Include an allocatable-specific error message
-; CHECK: Invalid register name global variable
+; CHECK: Trying to obtain non-reservable register "r2".
         %reg = call i64 @llvm.read_register.i64(metadata !0)
   ret i64 %reg
 }
diff --git a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2.ll b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2.ll
index ca79f857548ebe..ac79aa381ea7e0 100644
--- a/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2.ll
+++ b/llvm/test/CodeGen/PowerPC/named-reg-alloc-r2.ll
@@ -3,8 +3,7 @@
 
 define i32 @get_reg() nounwind {
 entry:
-; FIXME: Include an allocatable-specific error message
-; CHECK-NOTPPC32: Invalid register name global variable
+; CHECK-NOTPPC32: Trying to obtain non-reservable register "r2".
         %reg = call i32 @llvm.read_register.i32(metadata !0)
   ret i32 %reg
 

@lei137 lei137 requested a review from amy-kwan October 16, 2024 19:47
Copy link

github-actions bot commented Oct 16, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@lei137 lei137 marked this pull request as draft October 16, 2024 21:42
@lei137 lei137 self-assigned this Oct 16, 2024
@lei137 lei137 marked this pull request as ready for review October 16, 2024 22:09
@lei137 lei137 requested a review from redstar October 18, 2024 16:26
@lei137 lei137 force-pushed the lei/getRegisterByName branch from fe6ee6e to 9ff95f6 Compare October 18, 2024 18:05
Copy link
Member

@redstar redstar left a comment

Choose a reason for hiding this comment

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

LGTM.

@lei137 lei137 force-pushed the lei/getRegisterByName branch from 9ff95f6 to 78536f1 Compare October 22, 2024 18:33
@lei137 lei137 merged commit 06d1929 into llvm:main Oct 22, 2024
4 of 5 checks passed
@lei137 lei137 deleted the lei/getRegisterByName branch October 22, 2024 18:34
@nathanchance
Copy link
Member

For what it's worth, this breaks building the Linux kernel for 32-bit powerpc for me because the kernel uses r2 as a global register variable for current.

$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- LLVM=1 LLVM_IAS=0 mrproper pmac32_defconfig ipc/namespace.o
fatal error: error in backend: Trying to obtain a reserved register "r2".
...

While this seems expected given the change, just in case, I ran this through cvise, which spits out:

register struct task_struct *current asm("r2");
struct task_struct {
  struct cred *cred;
};
struct cred {
  int euid;
} inc_ucount(int);
int *create_ipc_ns() {
  inc_ucount(({ ({ current->cred; })->euid; }));
  return (void *)8;
}

which works with clang immediately prior to this change and GCC:

$ clang --version | head -1
ClangBuiltLinux clang version 20.0.0git (https://github.com/llvm/llvm-project.git 06d192925d3510d0af6c10e6f64f6deabf66b75f)

$ clang --target=powerpc-linux-gnu -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i
fatal error: error in backend: Trying to obtain a reserved register "r2".
...
$ clang --version | head -1
ClangBuiltLinux clang version 20.0.0git (https://github.com/llvm/llvm-project.git e57548387000071562f44bfd66644480c8e6542d)

$ clang --target=powerpc-linux-gnu -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i
$ powerpc-linux-gcc --version | head -1
powerpc-linux-gcc (GCC) 14.2.0

$ powerpc-linux-gcc -O2 -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i

@lei137
Copy link
Contributor Author

lei137 commented Oct 23, 2024

For what it's worth, this breaks building the Linux kernel for 32-bit powerpc for me because the kernel uses r2 as a global register variable for current.

$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- LLVM=1 LLVM_IAS=0 mrproper pmac32_defconfig ipc/namespace.o
fatal error: error in backend: Trying to obtain a reserved register "r2".
...

While this seems expected given the change, just in case, I ran this through cvise, which spits out:

register struct task_struct *current asm("r2");
struct task_struct {
  struct cred *cred;
};
struct cred {
  int euid;
} inc_ucount(int);
int *create_ipc_ns() {
  inc_ucount(({ ({ current->cred; })->euid; }));
  return (void *)8;
}

which works with clang immediately prior to this change and GCC:

$ clang --version | head -1
ClangBuiltLinux clang version 20.0.0git (https://github.com/llvm/llvm-project.git 06d192925d3510d0af6c10e6f64f6deabf66b75f)

$ clang --target=powerpc-linux-gnu -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i
fatal error: error in backend: Trying to obtain a reserved register "r2".
...
$ clang --version | head -1
ClangBuiltLinux clang version 20.0.0git (https://github.com/llvm/llvm-project.git e57548387000071562f44bfd66644480c8e6542d)

$ clang --target=powerpc-linux-gnu -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i
$ powerpc-linux-gcc --version | head -1
powerpc-linux-gcc (GCC) 14.2.0

$ powerpc-linux-gcc -O2 -Wall -Werror -Wfatal-errors -c -o /dev/null namespace.i

Thanks for reporting this. I will revert this change to unblock you and investigate further.

@lei137
Copy link
Contributor Author

lei137 commented Oct 23, 2024

@nathanchance I took another look at the docs and there is no documented restrictions on what register can be used for this. I updated the patch to reflect what was originally there for r2. Can youn please give it a try?
#113482

Thanks!

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
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.

4 participants