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

[AArch64] Remove unused ReverseCSRRestoreSeq option. #82326

Merged

Conversation

sdesmalen-arm
Copy link
Collaborator

This patch removes the -reverse-csr-restore-seq option from AArch64FrameLowering, since this is no longer used.

This patch removes the `-reverse-csr-restore-seq` option from
AArch64FrameLowering, since this is no longer used.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 20, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Sander de Smalen (sdesmalen-arm)

Changes

This patch removes the -reverse-csr-restore-seq option from AArch64FrameLowering, since this is no longer used.


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

2 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64FrameLowering.cpp (+21-45)
  • (removed) llvm/test/CodeGen/AArch64/reverse-csr-restore-seq.mir (-101)
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index 0e9adde564b3e5..c013bbe9926fef 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -239,11 +239,6 @@ static cl::opt<bool> EnableRedZone("aarch64-redzone",
                                    cl::desc("enable use of redzone on AArch64"),
                                    cl::init(false), cl::Hidden);
 
-static cl::opt<bool>
-    ReverseCSRRestoreSeq("reverse-csr-restore-seq",
-                         cl::desc("reverse the CSR restore sequence"),
-                         cl::init(false), cl::Hidden);
-
 static cl::opt<bool> StackTaggingMergeSetTag(
     "stack-tagging-merge-settag",
     cl::desc("merge settag instruction in function epilog"), cl::init(true),
@@ -307,8 +302,6 @@ bool AArch64FrameLowering::homogeneousPrologEpilog(
     return false;
   if (!EnableHomogeneousPrologEpilog)
     return false;
-  if (ReverseCSRRestoreSeq)
-    return false;
   if (EnableRedZone)
     return false;
 
@@ -3111,7 +3104,27 @@ bool AArch64FrameLowering::restoreCalleeSavedRegisters(
 
   computeCalleeSaveRegisterPairs(MF, CSI, TRI, RegPairs, hasFP(MF));
 
-  auto EmitMI = [&](const RegPairInfo &RPI) -> MachineBasicBlock::iterator {
+  if (homogeneousPrologEpilog(MF, &MBB)) {
+    auto MIB = BuildMI(MBB, MBBI, DL, TII.get(AArch64::HOM_Epilog))
+                   .setMIFlag(MachineInstr::FrameDestroy);
+    for (auto &RPI : RegPairs) {
+      MIB.addReg(RPI.Reg1, RegState::Define);
+      MIB.addReg(RPI.Reg2, RegState::Define);
+    }
+    return true;
+  }
+
+  // For performance reasons restore SVE register in increasing order
+  auto IsPPR = [](const RegPairInfo &c) { return c.Type == RegPairInfo::PPR; };
+  auto PPRBegin = std::find_if(RegPairs.begin(), RegPairs.end(), IsPPR);
+  auto PPREnd = std::find_if(RegPairs.rbegin(), RegPairs.rend(), IsPPR);
+  std::reverse(PPRBegin, PPREnd.base());
+  auto IsZPR = [](const RegPairInfo &c) { return c.Type == RegPairInfo::ZPR; };
+  auto ZPRBegin = std::find_if(RegPairs.begin(), RegPairs.end(), IsZPR);
+  auto ZPREnd = std::find_if(RegPairs.rbegin(), RegPairs.rend(), IsZPR);
+  std::reverse(ZPRBegin, ZPREnd.base());
+
+  for (const RegPairInfo &RPI : RegPairs) {
     unsigned Reg1 = RPI.Reg1;
     unsigned Reg2 = RPI.Reg2;
 
@@ -3185,43 +3198,6 @@ bool AArch64FrameLowering::restoreCalleeSavedRegisters(
         MachineMemOperand::MOLoad, Size, Alignment));
     if (NeedsWinCFI)
       InsertSEH(MIB, TII, MachineInstr::FrameDestroy);
-
-    return MIB->getIterator();
-  };
-
-  if (homogeneousPrologEpilog(MF, &MBB)) {
-    auto MIB = BuildMI(MBB, MBBI, DL, TII.get(AArch64::HOM_Epilog))
-                   .setMIFlag(MachineInstr::FrameDestroy);
-    for (auto &RPI : RegPairs) {
-      MIB.addReg(RPI.Reg1, RegState::Define);
-      MIB.addReg(RPI.Reg2, RegState::Define);
-    }
-    return true;
-  }
-
-  // For performance reasons restore SVE register in increasing order
-  auto IsPPR = [](const RegPairInfo &c) { return c.Type == RegPairInfo::PPR; };
-  auto PPRBegin = std::find_if(RegPairs.begin(), RegPairs.end(), IsPPR);
-  auto PPREnd = std::find_if(RegPairs.rbegin(), RegPairs.rend(), IsPPR);
-  std::reverse(PPRBegin, PPREnd.base());
-  auto IsZPR = [](const RegPairInfo &c) { return c.Type == RegPairInfo::ZPR; };
-  auto ZPRBegin = std::find_if(RegPairs.begin(), RegPairs.end(), IsZPR);
-  auto ZPREnd = std::find_if(RegPairs.rbegin(), RegPairs.rend(), IsZPR);
-  std::reverse(ZPRBegin, ZPREnd.base());
-
-  if (ReverseCSRRestoreSeq) {
-    MachineBasicBlock::iterator First = MBB.end();
-    for (const RegPairInfo &RPI : reverse(RegPairs)) {
-      MachineBasicBlock::iterator It = EmitMI(RPI);
-      if (First == MBB.end())
-        First = It;
-    }
-    if (First != MBB.end())
-      MBB.splice(MBBI, &MBB, First);
-  } else {
-    for (const RegPairInfo &RPI : RegPairs) {
-      (void)EmitMI(RPI);
-    }
   }
 
   return true;
diff --git a/llvm/test/CodeGen/AArch64/reverse-csr-restore-seq.mir b/llvm/test/CodeGen/AArch64/reverse-csr-restore-seq.mir
deleted file mode 100644
index de4baec50e0c66..00000000000000
--- a/llvm/test/CodeGen/AArch64/reverse-csr-restore-seq.mir
+++ /dev/null
@@ -1,101 +0,0 @@
-# RUN: llc -run-pass=prologepilog -reverse-csr-restore-seq -o - -mtriple=aarch64-- %s | FileCheck %s --check-prefixes=CHECK
-# RUN: llc -start-before=prologepilog -stop-after=aarch64-ldst-opt -reverse-csr-restore-seq -o - -mtriple=aarch64-- %s | FileCheck %s --check-prefixes=CHECK
-#
---- |
-
-  define void @foo() nounwind { entry: unreachable }
-
-  define void @bar() nounwind { entry: unreachable }
-
-  define void @baz() nounwind { entry: unreachable }
-
-...
----
-name:            foo
-# CHECK-LABEL: name: foo
-tracksRegLiveness: true
-body:             |
-  bb.0:
-    $x19 = IMPLICIT_DEF
-    $x20 = IMPLICIT_DEF
-    $x21 = IMPLICIT_DEF
-    $x22 = IMPLICIT_DEF
-    $x23 = IMPLICIT_DEF
-    $x24 = IMPLICIT_DEF
-    $x25 = IMPLICIT_DEF
-    $x26 = IMPLICIT_DEF
-
-  ; The local stack size is 0, so the last ldp in the sequence will also
-  ; restore the stack.
-  ; CHECK: $x24, $x23 = frame-destroy LDPXi $sp, 2
-  ; CHECK-NEXT: $x22, $x21 = frame-destroy LDPXi $sp, 4
-  ; CHECK-NEXT: $x20, $x19 = frame-destroy LDPXi $sp, 6
-
-  ; The ldp and the stack increment get merged even before
-  ; the load-store optimizer.
-  ; CHECK-NEXT: early-clobber $sp, $x26, $x25 = frame-destroy LDPXpost $sp, 8
-
-    RET_ReallyLR
-...
----
-name:            bar
-# CHECK-LABEL: name: bar
-tracksRegLiveness: true
-stack:
-  - { id : 0, size: 8, alignment: 4,
-  stack-id: default, callee-saved-register: '', callee-saved-restored: true,
-  local-offset: -4, debug-info-variable: '', debug-info-expression: '',
-  debug-info-location: '' }
-
-body:             |
-  bb.0:
-    $x19 = IMPLICIT_DEF
-    $x20 = IMPLICIT_DEF
-    $x21 = IMPLICIT_DEF
-    $x22 = IMPLICIT_DEF
-    $x23 = IMPLICIT_DEF
-    $x24 = IMPLICIT_DEF
-    $x25 = IMPLICIT_DEF
-    $x26 = IMPLICIT_DEF
-
-  ; The local stack size is not 0, and we can combine the CSR stack size with
-  ; the local stack size. This results in rewriting the offsets for all the
-  ; save/restores and forbids us to merge the stack adjustment and the last pop.
-  ; In this case, there is no point of moving the first CSR pair at the end.
-  ; We do it anyway, as it's a small price to pay for the resulting
-  ; simplification in the epilogue emission code.
-  ; CHECK:      $x24, $x23 = frame-destroy LDPXi $sp, 4
-  ; CHECK-NEXT: $x22, $x21 = frame-destroy LDPXi $sp, 6
-  ; CHECK-NEXT: $x20, $x19 = frame-destroy LDPXi $sp, 8
-  ; CHECK-NEXT: $x26, $x25 = frame-destroy LDPXi $sp, 2
-  ; CHECK-NEXT: $sp = frame-destroy ADDXri $sp, 80, 0
-    RET_ReallyLR
-...
----
-# Check that the load from the offset 0 is moved at the end even when hasFP is
-# false.
-name:            baz
-# CHECK-LABEL: name: baz
-alignment:       4
-tracksRegLiveness: true
-frameInfo:
-  adjustsStack:    true
-  hasCalls:        true
-body:             |
-  bb.0:
-    successors: %bb.1
-
-    $x0 = IMPLICIT_DEF
-    $x20 = IMPLICIT_DEF
-    $x21 = IMPLICIT_DEF
-
-    ADJCALLSTACKDOWN 0, 0, implicit-def dead $sp, implicit $sp
-    BL @foo, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0
-    ADJCALLSTACKUP 0, 0, implicit-def dead $sp, implicit $sp
-    B %bb.1
-
-  bb.1:
-   ; CHECK: $x21, $x20 = frame-destroy LDPXi $sp, 2
-   ; CHECK-NEXT: early-clobber $sp, $lr = frame-destroy LDRXpost $sp, 32
-    RET_ReallyLR
-...

Copy link
Contributor

@CarolineConcatto CarolineConcatto left a comment

Choose a reason for hiding this comment

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

Thank you Sander!

Copy link
Collaborator

@francisvm francisvm left a comment

Choose a reason for hiding this comment

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

Thanks!

@sdesmalen-arm sdesmalen-arm merged commit 493f101 into llvm:main Feb 20, 2024
6 checks passed
CarolineConcatto added a commit that referenced this pull request Feb 20, 2024
Patch  3f0404a is breaking some debugs build so we cannot use the reverse here.

This reverts commit 493f101.
CarolineConcatto pushed a commit that referenced this pull request Feb 22, 2024
This patch removes the `-reverse-csr-restore-seq` option from
AArch64FrameLowering, since this is no longer used.

This patch was reverted because of a crash in PR#79623.
Merging it back as it was fixed in PR#82492.
@sdesmalen-arm sdesmalen-arm deleted the remove-reverse-csr-restore-seq branch February 23, 2024 11:34
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.

None yet

4 participants