Skip to content

[LoongArch] Prevent R0/R1 allocation for rj operand of [G]CSRXCHG #140862

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

heiher
Copy link
Member

@heiher heiher commented May 21, 2025

The [G]CSRXCHG instruction must not use R0 or R1 as the rj operand, as encoding rj as 0 or 1 will be interpreted as [G]CSRRD OR [G]CSRWR, respectively, rather than [G]CSRXCHG.

This patch introduces a new register class GPRNoR0R1 and updates the [G]CSRXCHG instruction definition to use it for the rj operand, ensuring the register allocator avoids assigning R0 or R1.

Fixes #140842

The {G}CSRXCHG instruction must not use R0 or R1 as the rj operand, as
encoding rj as 0 or 1 will be interpreted as {G}CSRRD OR {G}CSRWR,
respectively, rather than {G}CSRXCHG.

This patch introduces a new register class `GPRNoR0R1` and updates the
{G}CSRXCHG instruction definition to use it for the rj operand, ensuring
the register allocator avoids assigning R0 or R1.

Fixes llvm#140842
@llvmbot llvmbot added mc Machine (object) code backend:loongarch labels May 21, 2025
@llvmbot
Copy link
Member

llvmbot commented May 21, 2025

@llvm/pr-subscribers-backend-loongarch

@llvm/pr-subscribers-mc

Author: hev (heiher)

Changes

The {G}CSRXCHG instruction must not use R0 or R1 as the rj operand, as encoding rj as 0 or 1 will be interpreted as {G}CSRRD OR {G}CSRWR, respectively, rather than {G}CSRXCHG.

This patch introduces a new register class GPRNoR0R1 and updates the {G}CSRXCHG instruction definition to use it for the rj operand, ensuring the register allocator avoids assigning R0 or R1.

Fixes #140842


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

7 Files Affected:

  • (modified) llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp (+8)
  • (modified) llvm/lib/Target/LoongArch/LoongArchInstrInfo.td (+3-3)
  • (modified) llvm/lib/Target/LoongArch/LoongArchLVZInstrInfo.td (+1-1)
  • (modified) llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td (+5)
  • (added) llvm/test/CodeGen/LoongArch/csrxchg-intrinsic.ll (+24)
  • (modified) llvm/test/MC/LoongArch/Basic/Privilege/invalid.s (+2-2)
  • (modified) llvm/test/MC/LoongArch/lvz/lvz-err.s (+2-2)
diff --git a/llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp b/llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp
index 5963208691f72..761682423fffe 100644
--- a/llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp
+++ b/llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp
@@ -62,6 +62,14 @@ static DecodeStatus DecodeGPRRegisterClass(MCInst &Inst, uint64_t RegNo,
   return MCDisassembler::Success;
 }
 
+static DecodeStatus
+DecodeGPRNoR0R1RegisterClass(MCInst &Inst, uint64_t RegNo, uint64_t Address,
+                             const MCDisassembler *Decoder) {
+  if (RegNo <= 1)
+    return MCDisassembler::Fail;
+  return DecodeGPRRegisterClass(Inst, RegNo, Address, Decoder);
+}
+
 static DecodeStatus DecodeFPR32RegisterClass(MCInst &Inst, uint64_t RegNo,
                                              uint64_t Address,
                                              const MCDisassembler *Decoder) {
diff --git a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
index e0fb49698383c..c6e361dffee9a 100644
--- a/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
+++ b/llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
@@ -2431,7 +2431,7 @@ let hasSideEffects = 1, Constraints = "$rd = $dst" in {
 def CSRWR : FmtCSR<0x04000020, (outs GPR:$dst),
                    (ins GPR:$rd, uimm14:$csr_num), "$rd, $csr_num">;
 def CSRXCHG : FmtCSRXCHG<0x04000000, (outs GPR:$dst),
-                         (ins GPR:$rd, GPR:$rj, uimm14:$csr_num),
+                         (ins GPR:$rd, GPRNoR0R1:$rj, uimm14:$csr_num),
                          "$rd, $rj, $csr_num">;
 } // hasSideEffects = 1, Constraints = "$rd = $dst"
 
@@ -2478,8 +2478,8 @@ def IDLE : MISC_I15<0x06488000>;
 def : Pat<(loongarch_csrrd uimm14:$imm14), (CSRRD uimm14:$imm14)>;
 def : Pat<(loongarch_csrwr GPR:$rd, uimm14:$imm14),
           (CSRWR GPR:$rd, uimm14:$imm14)>;
-def : Pat<(loongarch_csrxchg GPR:$rd, GPR:$rj, uimm14:$imm14),
-          (CSRXCHG GPR:$rd, GPR:$rj, uimm14:$imm14)>;
+def : Pat<(loongarch_csrxchg GPR:$rd, GPRNoR0R1:$rj, uimm14:$imm14),
+          (CSRXCHG GPR:$rd, GPRNoR0R1:$rj, uimm14:$imm14)>;
 
 def : Pat<(loongarch_iocsrrd_b GPR:$rj), (IOCSRRD_B GPR:$rj)>;
 def : Pat<(loongarch_iocsrrd_h GPR:$rj), (IOCSRRD_H GPR:$rj)>;
diff --git a/llvm/lib/Target/LoongArch/LoongArchLVZInstrInfo.td b/llvm/lib/Target/LoongArch/LoongArchLVZInstrInfo.td
index 50a16e2dd56b9..07b77ee971f27 100644
--- a/llvm/lib/Target/LoongArch/LoongArchLVZInstrInfo.td
+++ b/llvm/lib/Target/LoongArch/LoongArchLVZInstrInfo.td
@@ -23,7 +23,7 @@ let Constraints = "$rd = $dst" in {
 def GCSRWR : FmtCSR<0x05000020, (outs GPR:$dst),
                     (ins GPR:$rd, uimm14:$csr_num), "$rd, $csr_num">;
 def GCSRXCHG : FmtCSRXCHG<0x05000000, (outs GPR:$dst),
-                          (ins GPR:$rd, GPR:$rj, uimm14:$csr_num),
+                          (ins GPR:$rd, GPRNoR0R1:$rj, uimm14:$csr_num),
                           "$rd, $rj, $csr_num">;
 } // Constraints = "$rd = $dst"
 
diff --git a/llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td b/llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
index a8419980868ee..dfba2ecd02154 100644
--- a/llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
+++ b/llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td
@@ -127,6 +127,11 @@ def GPRT : GPRRegisterClass<(add // a0...a7, t0...t8
 // prediction.
 def GPRJR : GPRRegisterClass<(sub GPR, R1)>;
 
+// Don't use R0 or R1 for the rj operand of {G}CSRXCHG, because when rj is
+// encoded as 0 or 1, the instruction is interpreted as {G}CSRRD or {G}CSRWR,
+// respectively, rather than {G}CSRXCHG.
+def GPRNoR0R1 : GPRRegisterClass<(sub GPR, R0, R1)>;
+
 // Floating point registers
 
 let RegAltNameIndices = [RegAliasName] in {
diff --git a/llvm/test/CodeGen/LoongArch/csrxchg-intrinsic.ll b/llvm/test/CodeGen/LoongArch/csrxchg-intrinsic.ll
new file mode 100644
index 0000000000000..2f38b3a8c7ad1
--- /dev/null
+++ b/llvm/test/CodeGen/LoongArch/csrxchg-intrinsic.ll
@@ -0,0 +1,24 @@
+; RUN: llc --mtriple=loongarch32 --mattr=+f --verify-machineinstrs < %s | FileCheck %s
+; RUN: llc --mtriple=loongarch64 --mattr=+f --verify-machineinstrs < %s | FileCheck %s
+
+declare i32 @llvm.loongarch.csrxchg.w(i32, i32, i32 immarg)
+
+;; Check that the rj operand of csrxchg is not R0.
+define void @csrxchg_w_rj_not_r0(i32 signext %a) {
+; CHECK-NOT:    csrxchg ${{[a-z]*}}, $r0, 0
+; CHECK-NOT:    csrxchg ${{[a-z]*}}, $zero, 0
+entry:
+  %0 = tail call i32 @llvm.loongarch.csrxchg.w(i32 %a, i32 0, i32 0)
+  ret void
+}
+
+;; Check that the rj operand of csrxchg is not R1.
+define i32 @csrxchg_w_rj_not_r1() {
+; CHECK-NOT:    csrxchg ${{[a-z]*}}, $r1, 0
+; CHECK-NOT:    csrxchg ${{[a-z]*}}, $ra, 0
+entry:
+  %0 = tail call i32 asm "", "=r,r,i,{r4},{r5},{r6},{r7},{r8},{r9},{r10},{r11},{r12},{r13},{r14},{r15},{r16},{r17},{r18},{r19},{r20},{r23},{r24},{r25},{r26},{r27},{r28},{r29},{r30},{r31},0"(i32 4, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  %1 = tail call i32 @llvm.loongarch.csrxchg.w(i32 %0, i32 4, i32 0)
+  %2 = tail call i32 asm "", "=r,r,i,{r4},{r5},{r6},{r7},{r8},{r9},{r10},{r11},{r12},{r13},{r14},{r15},{r16},{r17},{r18},{r19},{r20},{r23},{r24},{r25},{r26},{r27},{r28},{r29},{r30},{r31},0"(i32 4, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 %1)
+  ret i32 %2
+}
diff --git a/llvm/test/MC/LoongArch/Basic/Privilege/invalid.s b/llvm/test/MC/LoongArch/Basic/Privilege/invalid.s
index 80d7c304956c9..2ac3fcf6e1819 100644
--- a/llvm/test/MC/LoongArch/Basic/Privilege/invalid.s
+++ b/llvm/test/MC/LoongArch/Basic/Privilege/invalid.s
@@ -3,9 +3,9 @@
 
 ## csrxchg: rj != 0,1
 csrxchg $a0, $zero, 0
-# ERR: :[[#@LINE-1]]:15: error: must not be $r0 or $r1
+# ERR: :[[#@LINE-1]]:15: error: invalid operand for instruction
 csrxchg $a0, $ra, 0
-# ERR: :[[#@LINE-1]]:15: error: must not be $r0 or $r1
+# ERR: :[[#@LINE-1]]:15: error: invalid operand for instruction
 
 ## LoongArch64 mnemonics
 iocsrrd.d $a0, $a1
diff --git a/llvm/test/MC/LoongArch/lvz/lvz-err.s b/llvm/test/MC/LoongArch/lvz/lvz-err.s
index 64b47a1fca6d5..b4ecb9d605edb 100644
--- a/llvm/test/MC/LoongArch/lvz/lvz-err.s
+++ b/llvm/test/MC/LoongArch/lvz/lvz-err.s
@@ -19,10 +19,10 @@ gcsrxchg $a0, $a1, -1
 # CHECK: :[[#@LINE-1]]:20: error: immediate must be an integer in the range [0, 16383]
 
 gcsrxchg $a0, $ra, 1
-# CHECK: :[[#@LINE-1]]:16: error: must not be $r0 or $r1
+# CHECK: :[[#@LINE-1]]:16: error: invalid operand for instruction
 
 gcsrxchg $a0, $zero, 1
-# CHECK: :[[#@LINE-1]]:16: error: must not be $r0 or $r1
+# CHECK: :[[#@LINE-1]]:16: error: invalid operand for instruction
 
 hvcl 32768
 # CHECK: :[[#@LINE-1]]:6: error: immediate must be an integer in the range [0, 32767]

Copy link
Contributor

@xen0n xen0n left a comment

Choose a reason for hiding this comment

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

Nice catch!

@@ -3,9 +3,9 @@

## csrxchg: rj != 0,1
csrxchg $a0, $zero, 0
# ERR: :[[#@LINE-1]]:15: error: must not be $r0 or $r1
# ERR: :[[#@LINE-1]]:15: error: invalid operand for instruction
Copy link
Contributor

Choose a reason for hiding this comment

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

You can have this:

diff --git a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
index 39c5e034f2a4..81e8c3657d5c 100644
--- a/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
+++ b/llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp
@@ -1633,6 +1633,9 @@ LoongArchAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
     return Match_Success;
   }
 
+  if (Kind == MCK_GPRNoR0R1 && (Reg == LoongArch::R0 || Reg == LoongArch::R1))
+    return Match_RequiresOpnd2NotR0R1;
+
   return Match_InvalidOperand;
 }
 

So that we can have the nice diagnostics here intact.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea.

@xen0n
Copy link
Contributor

xen0n commented May 21, 2025

Also for proper shell-like expansion you would want {,G}CSRXCHG (notice the comma).

$ echo {,G}CSRXCHG
CSRXCHG GCSRXCHG
$ echo {G}CSRXCHG
{G}CSRXCHG

@heiher
Copy link
Member Author

heiher commented May 22, 2025

Also for proper shell-like expansion you would want {,G}CSRXCHG (notice the comma).

$ echo {,G}CSRXCHG
CSRXCHG GCSRXCHG
$ echo {G}CSRXCHG
{G}CSRXCHG

Thanks! {,G}CSRXCHG is indeed valid for shell-style expansion, but here the intent is mainly documentation/readability and grep friendliness in comments. {G}CSRXCHG is more readable and still clearly expresses both forms, without implying literal brace expansion.

@xen0n
Copy link
Contributor

xen0n commented May 22, 2025

Also for proper shell-like expansion you would want {,G}CSRXCHG (notice the comma).

$ echo {,G}CSRXCHG
CSRXCHG GCSRXCHG
$ echo {G}CSRXCHG
{G}CSRXCHG

Thanks! {,G}CSRXCHG is indeed valid for shell-style expansion, but here the intent is mainly documentation/readability and grep friendliness in comments. {G}CSRXCHG is more readable and still clearly expresses both forms, without implying literal brace expansion.

Hmm then maybe "(G)CSRXCHG" with parens instead of braces?

@heiher
Copy link
Member Author

heiher commented May 22, 2025

Also for proper shell-like expansion you would want {,G}CSRXCHG (notice the comma).

$ echo {,G}CSRXCHG
CSRXCHG GCSRXCHG
$ echo {G}CSRXCHG
{G}CSRXCHG

Thanks! {,G}CSRXCHG is indeed valid for shell-style expansion, but here the intent is mainly documentation/readability and grep friendliness in comments. {G}CSRXCHG is more readable and still clearly expresses both forms, without implying literal brace expansion.

Hmm then maybe "(G)CSRXCHG" with parens instead of braces?

Perhaps it's better to use [G]CSRXCHG instead, as this aligns with the style used in the LoongArch ISA Manual.

@heiher heiher changed the title [LoongArch] Prevent R0/R1 allocation for rj operand of {G}CSRXCHG [LoongArch] Prevent R0/R1 allocation for rj operand of [G]CSRXCHG May 22, 2025
heiher added a commit to heiher/llvm-project that referenced this pull request May 22, 2025
This patch adds support for the `q` constraint:
a general-purpose register except for $r0 and $r1 (for the csrxchg
instruction)

Based on llvm#140862
Link: https://gcc.gnu.org/pipermail/gcc-patches/2025-May/684339.html
Copy link
Contributor

@xen0n xen0n 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:loongarch mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LoongArch] Incorrect register allocation for [G]CSRXCHG - rj must not be R0 ro R1
3 participants