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] provide CFI for ELF32 to unwind cr2, cr3, cr4 #83098

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

kernigh
Copy link
Contributor

@kernigh kernigh commented Feb 27, 2024

Delete the code that skips the CFI for the condition register on ELF32. The code checked !MustSaveCR, which happened only when Subtarget.is32BitELFABI(), where spillCalleeSavedRegisters is spilling cr in a different way. The spill was missing CFI. After deleting this code, a spill of cr2 to cr4 gets CFI in the same way as a spill of r14 to r31.

Fixes #83094

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 27, 2024

@llvm/pr-subscribers-backend-powerpc

Author: George Koehler (kernigh)

Changes

Delete the code that skips the CFI for the condition register on ELF32. The code checked !MustSaveCR, which happened only when Subtarget.is32BitELFABI(), where spillCalleeSavedRegisters is spilling cr in a different way. The spill was missing CFI. After deleting this code, a spill of cr2 to cr4 gets CFI in the same way as a spill of r14 to r31.

Fixes #83094


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

2 Files Affected:

  • (modified) llvm/lib/Target/PowerPC/PPCFrameLowering.cpp (-6)
  • (modified) llvm/test/CodeGen/PowerPC/crsave.ll (+13-9)
diff --git a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
index 6792842f8550c1..424501c35c043c 100644
--- a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
@@ -1191,12 +1191,6 @@ void PPCFrameLowering::emitPrologue(MachineFunction &MF,
       if ((Reg == PPC::X2 || Reg == PPC::R2) && MustSaveTOC)
         continue;
 
-      // For SVR4, don't emit a move for the CR spill slot if we haven't
-      // spilled CRs.
-      if (isSVR4ABI && (PPC::CR2 <= Reg && Reg <= PPC::CR4)
-          && !MustSaveCR)
-        continue;
-
       // For 64-bit SVR4 when we have spilled CRs, the spill location
       // is SP+8, not a frame-relative slot.
       if (isSVR4ABI && isPPC64 && (PPC::CR2 <= Reg && Reg <= PPC::CR4)) {
diff --git a/llvm/test/CodeGen/PowerPC/crsave.ll b/llvm/test/CodeGen/PowerPC/crsave.ll
index 81e7a0adcc8ca1..efa8b3f0120998 100644
--- a/llvm/test/CodeGen/PowerPC/crsave.ll
+++ b/llvm/test/CodeGen/PowerPC/crsave.ll
@@ -15,11 +15,12 @@ entry:
 }
 
 ; PPC32-LABEL: test_cr2:
-; PPC32: stwu 1, -32(1)
-; PPC32: stw 31, 28(1)
+; PPC32: stwu 1, -[[#%u,AMT:]](1)
+; PPC32: stw 31, [[#AMT - 4]](1)
+; PPC32: .cfi_offset cr2, -[[#%u,DOWN:]]
 ; PPC32: mfcr 12
-; PPC32-NEXT: stw 12, 24(31)
-; PPC32: lwz 12, 24(31)
+; PPC32-NEXT: stw 12, [[#AMT - DOWN]](31)
+; PPC32: lwz 12, [[#AMT - DOWN]](31)
 ; PPC32-NEXT: mtocrf 32, 12
 
 ; PPC64: .cfi_startproc
@@ -34,7 +35,7 @@ entry:
 ; PPC64: mtocrf 32, 12
 ; PPC64: .cfi_endproc
 
-define i32 @test_cr234() nounwind {
+define i32 @test_cr234() nounwind uwtable {
 entry:
   %ret = alloca i32, align 4
   %0 = call i32 asm sideeffect "\0A\09mtcr $4\0A\09cmpw 2,$2,$1\0A\09cmpw 3,$2,$2\0A\09cmpw 4,$2,$3\0A\09mfcr $0", "=r,r,r,r,r,~{cr2},~{cr3},~{cr4}"(i32 1, i32 2, i32 3, i32 0) nounwind
@@ -45,11 +46,14 @@ entry:
 }
 
 ; PPC32-LABEL: test_cr234:
-; PPC32: stwu 1, -32(1)
-; PPC32: stw 31, 28(1)
+; PPC32: stwu 1, -[[#%u,AMT:]](1)
+; PPC32: stw 31, [[#AMT - 4]](1)
+; PPC32: .cfi_offset cr2, -[[#%u,DOWN:]]
+; PPC32: .cfi_offset cr3, -[[#DOWN]]
+; PPC32: .cfi_offset cr4, -[[#DOWN]]
 ; PPC32: mfcr 12
-; PPC32-NEXT: stw 12, 24(31)
-; PPC32: lwz 12, 24(31)
+; PPC32-NEXT: stw 12, [[#AMT - DOWN]](31)
+; PPC32: lwz 12, [[#AMT - DOWN]](31)
 ; PPC32-NEXT: mtocrf 32, 12
 ; PPC32-NEXT: mtocrf 16, 12
 ; PPC32-NEXT: mtocrf 8, 12

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

This looks correct to me. MustSaveCR does not reflect the situation that whether cr2 ~ cr4 is spilled or not for SVR4 32-bit. So we can not depend on MustSaveCR to decide whether the CFI instruction should be generated. And for SVR4 32-bit, the non volatile CRs are stored related to frame-related slot, just below the GPR save area, so it is OK we use the common CFI generation logic at line 1222 instead of a customization one like the one for SVR4 64-bit. For SVR4 64-bit, the non volatile CRs are saved at a fixed address.

I will add some PPC reviewers. Please wait for some days before you commit this.

Thanks very much for fixing this.

; PPC32: stwu 1, -32(1)
; PPC32: stw 31, 28(1)
; PPC32: stwu 1, -[[#%u,AMT:]](1)
; PPC32: stw 31, [[#AMT - 4]](1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

would you please rebase the case after change 3196005 ? Thanks.

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. I stopped adding CFI to test_cr234, because cloberAllNvCrField has similar CFI.

Delete the code that skips the CFI for the condition register on
ELF32.  The code checked !MustSaveCR, which happened only when
Subtarget.is32BitELFABI(), where spillCalleeSavedRegisters is spilling
cr in a different way.  The spill was missing CFI.  After deleting
this code, a spill of cr2 to cr4 gets CFI in the same way as a spill
of r14 to r31.

Fixes llvm#83094
@kernigh
Copy link
Contributor Author

kernigh commented Mar 2, 2024

I don't have commit access to LLVM, so I will need someone else to commit it.

@brad0 brad0 merged commit 6b70c5d into llvm:main Mar 3, 2024
3 of 4 checks passed
bob-beck pushed a commit to openbsd/ports that referenced this pull request Mar 4, 2024
clang -S was missing a line like ".cfi_offset cr2, -16" in functions
that spill cr2 (or cr3, cr4) to the stack.  This was breaking a few
C++ exceptions.  This fix adds the missing CFI.

llvm/llvm-project#83098

ok robert@ (maintainer)
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 23, 2024
Delete the code that skips the CFI for the condition register on ELF32.
The code checked !MustSaveCR, which happened only when
Subtarget.is32BitELFABI(), where spillCalleeSavedRegisters is spilling
cr in a different way. The spill was missing CFI. After deleting this
code, a spill of cr2 to cr4 gets CFI in the same way as a spill of r14
to r31.

Fixes llvm#83094

(cherry picked from commit 6b70c5d)
tstellar pushed a commit to llvmbot/llvm-project that referenced this pull request Mar 27, 2024
Delete the code that skips the CFI for the condition register on ELF32.
The code checked !MustSaveCR, which happened only when
Subtarget.is32BitELFABI(), where spillCalleeSavedRegisters is spilling
cr in a different way. The spill was missing CFI. After deleting this
code, a spill of cr2 to cr4 gets CFI in the same way as a spill of r14
to r31.

Fixes llvm#83094

(cherry picked from commit 6b70c5d)
@pointhex pointhex mentioned this pull request May 7, 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.

[PowerPC] missing CFI for ELF32 to unwind cr2, cr3, cr4
4 participants