Skip to content

[MachineOutliner][NFC] Remove unnecessary RepeatedSequenceLocs.clear() #106171

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 1 commit into from
Aug 28, 2024

Conversation

kyulee-com
Copy link
Contributor

@kyulee-com kyulee-com commented Aug 27, 2024

  • When getOutliningCandidateInfo() returns std::nullopt (meaning no OutlinedFunction is created), there is no need to clear the input argument, RepeatedSequenceLocs, as it's already being cleared in the main loop of findCandidates().
  • Replaced 2 by MinRepeats, which I missed from [MachineOutliner][NFC] Refactor #105398

@kyulee-com kyulee-com marked this pull request as ready for review August 27, 2024 22:01
@llvmbot
Copy link
Member

llvmbot commented Aug 27, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Kyungwoo Lee (kyulee-com)

Changes
  • When getOutliningCandidateInfo() returns std::nullopt (meaning no OutlinedFunction is created), there is no need to clear the input argument, RepeatedSequenceLocs, as it's already beaing cleared in the main loop of findCandidates().
  • Replaced 2 by MinRepeats, which I missed from [MachineOutliner][NFC] Refactor #105398

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

1 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+3-7)
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 1423dc4996c2ed..397c271083c721 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -8990,7 +8990,7 @@ AArch64InstrInfo::getOutliningCandidateInfo(
         NumBytesNoStackCalls <= RepeatedSequenceLocs.size() * 12) {
       RepeatedSequenceLocs = CandidatesWithoutStackFixups;
       FrameID = MachineOutlinerNoLRSave;
-      if (RepeatedSequenceLocs.size() < 2)
+      if (RepeatedSequenceLocs.size() < MinRepeats)
         return std::nullopt;
     } else {
       SetCandidateCallInfo(MachineOutlinerDefault, 12);
@@ -9051,10 +9051,8 @@ AArch64InstrInfo::getOutliningCandidateInfo(
     }
 
     // If we dropped all of the candidates, bail out here.
-    if (RepeatedSequenceLocs.size() < MinRepeats) {
-      RepeatedSequenceLocs.clear();
+    if (RepeatedSequenceLocs.size() < MinRepeats)
       return std::nullopt;
-    }
   }
 
   // Does every candidate's MBB contain a call? If so, then we might have a call
@@ -9079,10 +9077,8 @@ AArch64InstrInfo::getOutliningCandidateInfo(
 
     if (ModStackToSaveLR) {
       // We can't fix up the stack. Bail out.
-      if (!AllStackInstrsSafe) {
-        RepeatedSequenceLocs.clear();
+      if (!AllStackInstrsSafe)
         return std::nullopt;
-      }
 
       // Save + restore LR.
       NumBytesToCreateFrame += 8;

 - When `getOutliningCandidateInfo()` returns `std::nullopt` (meaning
no `OutlinedFunction` is created), there is no need to clear the input
argument, `RepeatedSequenceLocs`, as it's already beaing cleared in the
main loop of `findCandidates()`.
 - Replaced `2` by `MinRepeats` as I missed this instance from llvm#105398
Copy link
Contributor

@thevinster thevinster left a comment

Choose a reason for hiding this comment

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

Typo in the PR title: MachineOutlineer -> MachineOutliner

@kyulee-com kyulee-com changed the title [MachineOutlineer][NFC] Remove unnecessary RepeatedSequenceLocs.clear() [MachineOutliner][NFC] Remove unnecessary RepeatedSequenceLocs.clear() Aug 28, 2024
@kyulee-com
Copy link
Contributor Author

Updated the tittle per suggestion. Thanks for the quick review!

@kyulee-com kyulee-com merged commit 140381d into llvm:main Aug 28, 2024
8 checks passed
@kyulee-com kyulee-com deleted the nfcnull branch August 28, 2024 14:09
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.

3 participants