Skip to content

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: 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
tclin914 pushed a commit to tclin914/llvm-project that referenced this pull request Sep 5, 2025
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}.
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