Skip to content

[RISCV] Fix bugs about getting register list of Zcmp push/pop. #66073

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 5 commits into from
Sep 20, 2023

Conversation

yetingk
Copy link
Contributor

@yetingk yetingk commented Sep 12, 2023

The pr does two things, one is to fix internal compiler error when we need to spill callee saves but none of them is GPR, another is to fix wrong register number for pushed registers are {ra, s0-s11}.

The original code encounters ICE when we need to spill callee saves but none of
them is GPR.
@yetingk yetingk requested a review from Xinlong-Wu September 12, 2023 12:13
@yetingk yetingk requested a review from a team as a code owner September 12, 2023 12:13
@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2023

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

Changes

The original code encounters ICE when we need to spill callee saves but none of them is GPR.

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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+5-5)
  • (added) llvm/test/CodeGen/RISCV/zcmp-crash.ll (+21)
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index add933250f8473d..898412ff446b075 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -1371,12 +1371,12 @@ bool RISCVFrameLowering::spillCalleeSavedRegisters(
   RISCVMachineFunctionInfo *RVFI = MF->getInfo();
   if (RVFI->isPushable(*MF)) {
     Register MaxReg = getMaxPushPopReg(*MF, CSI);
-    unsigned PushedRegNum =
-        getPushPopEncoding(MaxReg) - llvm::RISCVZC::RLISTENCODE::RA + 1;
-    RVFI->setRVPushRegs(PushedRegNum);
-    RVFI->setRVPushStackSize(alignTo((STI.getXLen() / 8) * PushedRegNum, 16));
-
     if (MaxReg != RISCV::NoRegister) {
+      unsigned PushedRegNum =
+          getPushPopEncoding(MaxReg) - llvm::RISCVZC::RLISTENCODE::RA + 1;
+      RVFI->setRVPushRegs(PushedRegNum);
+      RVFI->setRVPushStackSize(alignTo((STI.getXLen() / 8) * PushedRegNum, 16));
+
       // Use encoded number to represent registers to spill.
       unsigned RegEnc = getPushPopEncoding(MaxReg);
       RVFI->setRVPushRlist(RegEnc);
diff --git a/llvm/test/CodeGen/RISCV/zcmp-crash.ll b/llvm/test/CodeGen/RISCV/zcmp-crash.ll
new file mode 100644
index 000000000000000..b22085f95a3d1b0
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/zcmp-crash.ll
@@ -0,0 +1,21 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc -mtriple=riscv32 -target-abi ilp32f -mattr=+f,+zcmp -verify-machineinstrs < %s | FileCheck %s
+
+; Test the file could be compiled successfully.
+define void @bar() {
+; CHECK-LABEL: bar:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    addi sp, sp, -16
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    fsw fs0, 12(sp) # 4-byte Folded Spill
+; CHECK-NEXT:    .cfi_offset fs0, -4
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    fmv.w.x fs0, zero
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    flw fs0, 12(sp) # 4-byte Folded Reload
+; CHECK-NEXT:    addi sp, sp, 16
+; CHECK-NEXT:    ret
+entry:
+  tail call void asm sideeffect "fmv.w.x fs0, zero", "~{fs0}"()
+  ret void
+}

@jrtc27
Copy link
Collaborator

jrtc27 commented Sep 12, 2023

ICE is GCC terminology

@yetingk
Copy link
Contributor Author

yetingk commented Sep 12, 2023

ICE is GCC terminology

Thank you. I replace it with "internal` compiler error".

@yetingk yetingk changed the title [RISCV] Fix internal compiler about Zcmp. [RISCV] Fix bugs about getting register list of Zcmp push/pop. Sep 14, 2023
@yetingk
Copy link
Contributor Author

yetingk commented Sep 14, 2023

I also adds another fix about register number of Zcmp push/pop.

Comment on lines 229 to 230
// Return encoded value for PUSH/POP instruction, representing
// registers to store/load.
Copy link
Member

Choose a reason for hiding this comment

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

Comment need to update to reflect the update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

; RV64IZCMP-WITH-FP-NEXT: addi sp, sp, 32
; RV64IZCMP-WITH-FP-NEXT: ret
entry:
tail call void asm sideeffect "li s4, 0", "~{s4}"()
tail call void asm sideeffect "li s11, 0", "~{s11}"()
Copy link
Member

Choose a reason for hiding this comment

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

Add one more function instead of updating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@asb asb left a comment

Choose a reason for hiding this comment

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

I believe this is correct, would appreciate another quick review from someone who's been more active in the other zcmp patches though.

@yetingk yetingk merged commit 976df42 into llvm:main Sep 20, 2023
@yetingk yetingk deleted the zcmp-fix branch January 2, 2024 15:00
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.

5 participants