[X86][IPRA] Add getIPRACSRegs since frame registers are risked to be optimized out. #109597

FreddyLeaf · 2024-09-23T02:16:07Z

X86 IPRA had below correctness issue: https://gcc.godbolt.org/z/6hh88xv9r
This patch is a workaround to fix it.

This patch is a workaround to fix the correctness of IPRA on X86.

llvmbot · 2024-09-23T02:16:39Z

@llvm/pr-subscribers-backend-x86

Author: Freddy Ye (FreddyLeaf)

Changes

This patch is a workaround to fix the correctness of IPRA on X86.

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

6 Files Affected:

(modified) llvm/include/llvm/CodeGen/TargetRegisterInfo.h (+4)
(modified) llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp (+5-1)
(modified) llvm/lib/Target/X86/X86CallingConv.td (+3)
(modified) llvm/lib/Target/X86/X86RegisterInfo.cpp (+5)
(modified) llvm/lib/Target/X86/X86RegisterInfo.h (+3)
(added) llvm/test/CodeGen/X86/ipra-local-linkage-2.ll (+37)

diff --git a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
index 1a2f31e199336a..0f6484fddfe61f 100644
--- a/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetRegisterInfo.h
@@ -489,6 +489,10 @@ class TargetRegisterInfo : public MCRegisterInfo {
   virtual const MCPhysReg*
   getCalleeSavedRegs(const MachineFunction *MF) const = 0;
 
+  /// Return a null-terminated list of all of the callee-saved registers on
+  /// this target when IPRA is on. Normally, this list should be null.
+  virtual const MCPhysReg *getIPRACSRegs(const MachineFunction *MF) const = 0;
+
   /// Return a mask of call-preserved registers for the given calling convention
   /// on the current function. The mask should include all call-preserved
   /// aliases. This is used by the register allocator to determine which
diff --git a/llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp b/llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp
index 7d054cb7c7c71f..364cc933731dec 100644
--- a/llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp
+++ b/llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp
@@ -107,8 +107,12 @@ void TargetFrameLowering::determineCalleeSaves(MachineFunction &MF,
   // are preferred over callee saved registers.
   if (MF.getTarget().Options.EnableIPRA &&
       isSafeForNoCSROpt(MF.getFunction()) &&
-      isProfitableForNoCSROpt(MF.getFunction()))
+      isProfitableForNoCSROpt(MF.getFunction())) {
+    const MCPhysReg *IPRACSRegs = TRI.getIPRACSRegs(&MF);
+    for (unsigned i = 0; IPRACSRegs[i]; ++i)
+      SavedRegs.set(IPRACSRegs[i]);
     return;
+  }
 
   // Get the callee saved register list...
   const MCPhysReg *CSRegs = MF.getRegInfo().getCalleeSavedRegs();
diff --git a/llvm/lib/Target/X86/X86CallingConv.td b/llvm/lib/Target/X86/X86CallingConv.td
index 307aeb2ea4c6fd..472823a6d036ba 100644
--- a/llvm/lib/Target/X86/X86CallingConv.td
+++ b/llvm/lib/Target/X86/X86CallingConv.td
@@ -1104,6 +1104,9 @@ def CC_X86 : CallingConv<[
 
 def CSR_NoRegs : CalleeSavedRegs<(add)>;
 
+def CSR_IPRA_32 : CalleeSavedRegs<(add EBP)>;
+def CSR_IPRA_64 : CalleeSavedRegs<(add RBP)>;
+
 def CSR_32 : CalleeSavedRegs<(add ESI, EDI, EBX, EBP)>;
 def CSR_64 : CalleeSavedRegs<(add RBX, R12, R13, R14, R15, RBP)>;
 
diff --git a/llvm/lib/Target/X86/X86RegisterInfo.cpp b/llvm/lib/Target/X86/X86RegisterInfo.cpp
index 1d8808f4e2b7d0..302d50581e1e6b 100644
--- a/llvm/lib/Target/X86/X86RegisterInfo.cpp
+++ b/llvm/lib/Target/X86/X86RegisterInfo.cpp
@@ -410,6 +410,11 @@ X86RegisterInfo::getCalleeSavedRegs(const MachineFunction *MF) const {
   return CallsEHReturn ? CSR_32EHRet_SaveList : CSR_32_SaveList;
 }
 
+const MCPhysReg *
+X86RegisterInfo::getIPRACSRegs(const MachineFunction *MF) const {
+  return Is64Bit ? CSR_IPRA_64_SaveList : CSR_IPRA_32_SaveList;
+}
+
 const MCPhysReg *X86RegisterInfo::getCalleeSavedRegsViaCopy(
     const MachineFunction *MF) const {
   assert(MF && "Invalid MachineFunction pointer.");
diff --git a/llvm/lib/Target/X86/X86RegisterInfo.h b/llvm/lib/Target/X86/X86RegisterInfo.h
index 2f73698a4b94d3..68ee372f27b14d 100644
--- a/llvm/lib/Target/X86/X86RegisterInfo.h
+++ b/llvm/lib/Target/X86/X86RegisterInfo.h
@@ -99,6 +99,9 @@ class X86RegisterInfo final : public X86GenRegisterInfo {
   /// callee-save registers on this target.
   const MCPhysReg *
   getCalleeSavedRegs(const MachineFunction* MF) const override;
+  /// getIPRACSRegs - This API can be removed when rbp is safe to optimized out
+  /// when IPRA is on.
+  const MCPhysReg *getIPRACSRegs(const MachineFunction *MF) const override;
   const MCPhysReg *
   getCalleeSavedRegsViaCopy(const MachineFunction *MF) const;
   const uint32_t *getCallPreservedMask(const MachineFunction &MF,
diff --git a/llvm/test/CodeGen/X86/ipra-local-linkage-2.ll b/llvm/test/CodeGen/X86/ipra-local-linkage-2.ll
new file mode 100644
index 00000000000000..f337dfc989d81a
--- /dev/null
+++ b/llvm/test/CodeGen/X86/ipra-local-linkage-2.ll
@@ -0,0 +1,37 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc -enable-ipra < %s | FileCheck %s
+
+; This test is to ensure rbp is correctly saved/restored before/after the
+; inline asm call in foo()
+
+target triple = "x86_64--"
+
+define internal void @foo() norecurse nounwind {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    pushq %rbp
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    xorl %ebp, %ebp
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    popq %rbp
+; CHECK-NEXT:    retq
+  call void asm sideeffect "xor %ebp, %ebp", "~{ebp}"()
+  ret void
+}
+
+define void @bar(i32 %X) "frame-pointer"="all" nounwind {
+; CHECK-LABEL: bar:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    pushq %rbp
+; CHECK-NEXT:    movq %rsp, %rbp
+; CHECK-NEXT:    subq $16, %rsp
+; CHECK-NEXT:    callq foo
+; CHECK-NEXT:    movl $5, -4(%rbp)
+; CHECK-NEXT:    addq $16, %rsp
+; CHECK-NEXT:    popq %rbp
+; CHECK-NEXT:    retq
+  call void @foo()
+  %addr = alloca i32, align 4
+  store i32 5, ptr %addr, align 4
+  ret void
+}

weiguozhi · 2024-09-23T17:48:59Z

X86 IPRA had below correctness issue: https://gcc.godbolt.org/z/6hh88xv9r This patch is a workaround to fix it.

The instructions for function foo is already broken even without considering IPRA. The RBP is not preserved according to the standard C calling convention.

rnk

I think this would benefit from a clearer explanation of the problem in a GitHub issue.

If I try to fill in the details myself, I guess the issue arises because LLVM doesn't use RBP the same way other callee-saved registers are used. We simply reserve it from the set of allocatable registers, and hard code its usage to address stack memory when hasFP is true.

This should generalize to cover base pointers as well (ESI / RBP), where you'll see the same problem if you construct a complicated enough test case.

Do the main IPRA stakeholders feel like this is an acceptable fix? It effectively prevents making RBP an IPRA allocatable register, which seems like it might not be acceptable.

FreddyLeaf · 2024-09-24T00:30:35Z

X86 IPRA had below correctness issue: https://gcc.godbolt.org/z/6hh88xv9r This patch is a workaround to fix it.

The instructions for function foo is already broken even without considering IPRA. The RBP is not preserved according to the standard C calling convention.

Emm. The rbp can be correctly preserved if we removed the option of -enable-ipra in the example, not sure if I understood you correctly.

FreddyLeaf · 2024-09-24T00:39:33Z

I think this would benefit from a clearer explanation of the problem in a GitHub issue.

If I try to fill in the details myself, I guess the issue arises because LLVM doesn't use RBP the same way other callee-saved registers are used. We simply reserve it from the set of allocatable registers, and hard code its usage to address stack memory when hasFP is true.

This should generalize to cover base pointers as well (ESI / RBP), where you'll see the same problem if you construct a complicated enough test case.

Do the main IPRA stakeholders feel like this is an acceptable fix? It effectively prevents making RBP an IPRA allocatable register, which seems like it might not be acceptable.

Good idea. I'll create a new issue with more details. For ESI/RSI, since they are originally not X86 Callee Saved Registers, so I think IPRA won't have opportunity to optimize its push/pop out in the leaf function. Currently X86 didn't enable IPRA on by default. This issue is found by running cpu2017 benchmarks with explicitly enabling it on. I'll try add some IPRA guys. If you know some active ones, pls help add as well :)

FreddyLeaf · 2024-09-24T01:24:56Z

Created a github issue: #109739

FreddyLeaf · 2024-09-24T03:19:38Z

This issue is mainly due to $rbp is still not assigned at the stage of PEI pass. Instead it's a frame index: %stack.0.addr. So the risked registers should not only be $rbp, but all possible physical frame registers. Will update.
And sorry for the wrong conclusion above, ESI is indeed x86 callee saved register.

FreddyLeaf · 2024-09-24T03:29:54Z

Though this patch is paying attention to IPRA optimizations on callee save operations, the main component of IPRA optimizations should be contributed from caller save operations :)

FreddyLeaf · 2024-09-24T08:55:38Z

need to fix CI, set as draft first

FreddyLeaf · 2024-09-25T01:44:10Z

ping for review

llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp

llvm/lib/Target/X86/X86CallingConv.td

FreddyLeaf · 2024-09-25T05:41:34Z

So the problemed code this PR is paying attention now looks like so: https://gcc.godbolt.org/z/Tr8G5b36z

rnk

I'm not a major IPRA stakeholder, so there might be a better owner, but I think correctness comes before performance tradeoffs, so we should go ahead and land this and work to optimize from a less buggy state. Thanks for the fix!

llvm/include/llvm/CodeGen/TargetRegisterInfo.h

KanRobert · 2024-09-26T01:38:52Z

Should we enable IPRA by default for x86 after this patch?

FreddyLeaf · 2024-09-26T01:45:31Z

Should we enable IPRA by default for x86 after this patch?

It depends on broader performance data, which I haven't got yet. We can consider it after then.

…optimized out. (llvm#109597) X86 IPRA had below correctness issue: https://gcc.godbolt.org/z/6hh88xv9r This patch is a workaround to fix it.

[X86][IPRA] Add getIPRACSRegs since rbp is risked to be optimized out.

8ada603

This patch is a workaround to fix the correctness of IPRA on X86.

llvmbot added the backend:X86 label Sep 23, 2024

FreddyLeaf mentioned this pull request Sep 23, 2024

[X86] Refine speed up checking clobbered FP/BP to make IPRA work. #109246

Closed

FreddyLeaf requested review from phoebewang and weiguozhi September 23, 2024 02:22

fix CI

12f3319

KanRobert self-requested a review September 23, 2024 05:52

FreddyLeaf requested a review from RKSimon September 23, 2024 07:23

weiguozhi requested a review from rnk September 23, 2024 17:46

rnk reviewed Sep 23, 2024

View reviewed changes

FreddyLeaf requested a review from joker-eph September 24, 2024 01:30

Add more X86 potential frame registers in CSR_IPRA_32/64

e00bcd9

FreddyLeaf changed the title ~~[X86][IPRA] Add getIPRACSRegs since rbp is risked to be optimized out.~~ [X86][IPRA] Add getIPRACSRegs since frame registers are risked to be optimized out. Sep 24, 2024

FreddyLeaf marked this pull request as draft September 24, 2024 08:55

fix CI, risked CSRegs also require check isPhysRegModified

d7e1ea3

FreddyLeaf marked this pull request as ready for review September 24, 2024 12:00

KanRobert reviewed Sep 25, 2024

View reviewed changes

llvm/lib/CodeGen/TargetFrameLoweringImpl.cpp Outdated Show resolved Hide resolved

KanRobert reviewed Sep 25, 2024

View reviewed changes

llvm/lib/Target/X86/X86CallingConv.td Show resolved Hide resolved

FreddyLeaf added 3 commits September 25, 2024 13:10

address comments

de92c7f

update test with nounwind

7beeba8

update test

98ec015

rnk approved these changes Sep 25, 2024

View reviewed changes

llvm/include/llvm/CodeGen/TargetRegisterInfo.h Outdated Show resolved Hide resolved

address comments

ef0f268

KanRobert approved these changes Sep 26, 2024

View reviewed changes

FreddyLeaf merged commit 14b567d into llvm:main Sep 26, 2024
8 checks passed

FreddyLeaf deleted the ipra_fix2 branch September 26, 2024 02:57

mikhailramalho mentioned this pull request Feb 10, 2025

[RISCV] Enable IPRA for RISC-V #126594

Open

8 tasks

[X86][IPRA] Add getIPRACSRegs since frame registers are risked to be optimized out. #109597

[X86][IPRA] Add getIPRACSRegs since frame registers are risked to be optimized out. #109597

Uh oh!

Conversation

FreddyLeaf commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 23, 2024

Uh oh!

weiguozhi commented Sep 23, 2024

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

FreddyLeaf commented Sep 24, 2024

Uh oh!

FreddyLeaf commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FreddyLeaf commented Sep 24, 2024

Uh oh!

FreddyLeaf commented Sep 24, 2024

Uh oh!

FreddyLeaf commented Sep 24, 2024

Uh oh!

FreddyLeaf commented Sep 24, 2024

Uh oh!

FreddyLeaf commented Sep 25, 2024

Uh oh!

Uh oh!

Uh oh!

FreddyLeaf commented Sep 25, 2024

Uh oh!

rnk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KanRobert commented Sep 26, 2024

Uh oh!

FreddyLeaf commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FreddyLeaf commented Sep 23, 2024 •

edited

Loading

FreddyLeaf commented Sep 24, 2024 •

edited

Loading

FreddyLeaf commented Sep 26, 2024 •

edited

Loading