[llvm] [BOLT] Ignore returns in DataAggregator (PR #90807)

Amir Ayupov via llvm-commits llvm-commits at lists.llvm.org
Wed May 8 12:02:18 PDT 2024


https://github.com/aaupov updated https://github.com/llvm/llvm-project/pull/90807

>From 2b1be3d4ca776d4fbcf6753078eec0ed268ff552 Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 1 May 2024 18:09:38 -0700
Subject: [PATCH 1/6] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?=
 =?UTF-8?q?anges=20to=20main=20this=20commit=20is=20based=20on?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Created using spr 1.3.4

[skip ci]
---
 bolt/include/bolt/Core/BinaryFunction.h   |  6 ++++++
 bolt/lib/Core/BinaryFunction.cpp          |  6 +-----
 bolt/lib/Passes/ValidateInternalCalls.cpp | 13 ++++---------
 bolt/lib/Rewrite/RewriteInstance.cpp      |  3 ++-
 4 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 26d2d01f86267..540a9767760ad 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -361,6 +361,9 @@ class BinaryFunction {
   /// True if another function body was merged into this one.
   bool HasFunctionsFoldedInto{false};
 
+  /// True if the function has internal calls.
+  bool HasInternalCalls{false};
+
   /// Name for the section this function code should reside in.
   std::string CodeSectionName;
 
@@ -1334,6 +1337,9 @@ class BinaryFunction {
   /// Return true if other functions were folded into this one.
   bool hasFunctionsFoldedInto() const { return HasFunctionsFoldedInto; }
 
+  /// Return true if the function has internal calls.
+  bool hasInternalCalls() const { return HasInternalCalls; }
+
   /// If this function was folded, return the function it was folded into.
   BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
 
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 1fa96dfaabde8..8b12492aa1a13 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1281,6 +1281,7 @@ Error BinaryFunction::disassemble() {
             // Recursive call.
             TargetSymbol = getSymbol();
           } else {
+            HasInternalCalls = true;
             if (BC.isX86()) {
               // Dangerous old-style x86 PIC code. We may need to freeze this
               // function, so preserve the function as is for now.
@@ -1436,11 +1437,6 @@ Error BinaryFunction::disassemble() {
 
   clearList(Relocations);
 
-  if (!IsSimple) {
-    clearList(Instructions);
-    return createNonFatalBOLTError("");
-  }
-
   updateState(State::Disassembled);
 
   return Error::success();
diff --git a/bolt/lib/Passes/ValidateInternalCalls.cpp b/bolt/lib/Passes/ValidateInternalCalls.cpp
index 88df2e5b59f38..24f9bfde401ae 100644
--- a/bolt/lib/Passes/ValidateInternalCalls.cpp
+++ b/bolt/lib/Passes/ValidateInternalCalls.cpp
@@ -309,15 +309,10 @@ Error ValidateInternalCalls::runOnFunctions(BinaryContext &BC) {
   std::set<BinaryFunction *> NeedsValidation;
   for (auto &BFI : BC.getBinaryFunctions()) {
     BinaryFunction &Function = BFI.second;
-    for (BinaryBasicBlock &BB : Function) {
-      for (MCInst &Inst : BB) {
-        if (getInternalCallTarget(Function, Inst)) {
-          NeedsValidation.insert(&Function);
-          Function.setSimple(false);
-          break;
-        }
-      }
-    }
+    if (!Function.hasInternalCalls())
+      continue;
+    NeedsValidation.insert(&Function);
+    Function.setSimple(false);
   }
 
   // Skip validation for non-relocation mode
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 23f79e3c135a7..f6f597c8cee0d 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3343,7 +3343,8 @@ void RewriteInstance::disassembleFunctions() {
 
     if (!Function.isSimple()) {
       assert((!BC->HasRelocations || Function.getSize() == 0 ||
-              Function.hasIndirectTargetToSplitFragment()) &&
+              Function.hasIndirectTargetToSplitFragment() ||
+              Function.hasInternalCalls()) &&
              "unexpected non-simple function in relocation mode");
       continue;
     }

>From f21c7c5579eaf8bfd481e2c87920b8e95f9c8ecc Mon Sep 17 00:00:00 2001
From: Nikita Popov <npopov at redhat.com>
Date: Thu, 2 May 2024 16:26:05 -0700
Subject: [PATCH 2/6] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?=
 =?UTF-8?q?anges=20introduced=20through=20rebase?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Created using spr 1.3.4

[skip ci]
---
 bolt/include/bolt/Core/BinaryFunction.h   |  6 ------
 bolt/lib/Core/BinaryFunction.cpp          |  6 +++++-
 bolt/lib/Passes/ValidateInternalCalls.cpp | 13 +++++++++----
 bolt/lib/Rewrite/RewriteInstance.cpp      |  3 +--
 4 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index 540a9767760ad..26d2d01f86267 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -361,9 +361,6 @@ class BinaryFunction {
   /// True if another function body was merged into this one.
   bool HasFunctionsFoldedInto{false};
 
-  /// True if the function has internal calls.
-  bool HasInternalCalls{false};
-
   /// Name for the section this function code should reside in.
   std::string CodeSectionName;
 
@@ -1337,9 +1334,6 @@ class BinaryFunction {
   /// Return true if other functions were folded into this one.
   bool hasFunctionsFoldedInto() const { return HasFunctionsFoldedInto; }
 
-  /// Return true if the function has internal calls.
-  bool hasInternalCalls() const { return HasInternalCalls; }
-
   /// If this function was folded, return the function it was folded into.
   BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
 
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 8b12492aa1a13..1fa96dfaabde8 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1281,7 +1281,6 @@ Error BinaryFunction::disassemble() {
             // Recursive call.
             TargetSymbol = getSymbol();
           } else {
-            HasInternalCalls = true;
             if (BC.isX86()) {
               // Dangerous old-style x86 PIC code. We may need to freeze this
               // function, so preserve the function as is for now.
@@ -1437,6 +1436,11 @@ Error BinaryFunction::disassemble() {
 
   clearList(Relocations);
 
+  if (!IsSimple) {
+    clearList(Instructions);
+    return createNonFatalBOLTError("");
+  }
+
   updateState(State::Disassembled);
 
   return Error::success();
diff --git a/bolt/lib/Passes/ValidateInternalCalls.cpp b/bolt/lib/Passes/ValidateInternalCalls.cpp
index 24f9bfde401ae..88df2e5b59f38 100644
--- a/bolt/lib/Passes/ValidateInternalCalls.cpp
+++ b/bolt/lib/Passes/ValidateInternalCalls.cpp
@@ -309,10 +309,15 @@ Error ValidateInternalCalls::runOnFunctions(BinaryContext &BC) {
   std::set<BinaryFunction *> NeedsValidation;
   for (auto &BFI : BC.getBinaryFunctions()) {
     BinaryFunction &Function = BFI.second;
-    if (!Function.hasInternalCalls())
-      continue;
-    NeedsValidation.insert(&Function);
-    Function.setSimple(false);
+    for (BinaryBasicBlock &BB : Function) {
+      for (MCInst &Inst : BB) {
+        if (getInternalCallTarget(Function, Inst)) {
+          NeedsValidation.insert(&Function);
+          Function.setSimple(false);
+          break;
+        }
+      }
+    }
   }
 
   // Skip validation for non-relocation mode
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index f6f597c8cee0d..23f79e3c135a7 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3343,8 +3343,7 @@ void RewriteInstance::disassembleFunctions() {
 
     if (!Function.isSimple()) {
       assert((!BC->HasRelocations || Function.getSize() == 0 ||
-              Function.hasIndirectTargetToSplitFragment() ||
-              Function.hasInternalCalls()) &&
+              Function.hasIndirectTargetToSplitFragment()) &&
              "unexpected non-simple function in relocation mode");
       continue;
     }

>From acf58ceb37d2aa917e8d84d243faadc58f5f3a7d Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Mon, 6 May 2024 13:35:04 -0700
Subject: [PATCH 3/6] Simplify IsReturn check

Created using spr 1.3.4
---
 bolt/lib/Profile/DataAggregator.cpp | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp
index e4a7324c38175..d02e4499014ed 100644
--- a/bolt/lib/Profile/DataAggregator.cpp
+++ b/bolt/lib/Profile/DataAggregator.cpp
@@ -778,13 +778,13 @@ bool DataAggregator::doBranch(uint64_t From, uint64_t To, uint64_t Count,
     if (BinaryFunction *Func = getBinaryFunctionContainingAddress(Addr)) {
       Addr -= Func->getAddress();
       if (IsFrom) {
-        if (Func->hasInstructions()) {
-          if (MCInst *Inst = Func->getInstructionAtOffset(Addr))
-            IsReturn = BC->MIB->isReturn(*Inst);
-        } else if (std::optional<MCInst> Inst =
-                Func->disassembleInstructionAtOffset(Addr)) {
-          IsReturn = BC->MIB->isReturn(*Inst);
-        }
+        auto checkReturn = [&](auto MaybeInst) {
+          IsReturn = MaybeInst && BC->MIB->isReturn(*MaybeInst);
+        };
+        if (Func->hasInstructions())
+          checkReturn(Func->getInstructionAtOffset(Addr));
+        else
+          checkReturn(Func->disassembleInstructionAtOffset(Addr));
       }
 
       if (BAT)

>From 22052e461e5671f376fe2dcb733446b0a63e956d Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Tue, 7 May 2024 18:30:48 -0700
Subject: [PATCH 4/6] drop const from disassembleInstructionAtOffset

Created using spr 1.3.4
---
 bolt/include/bolt/Core/BinaryFunction.h | 3 +--
 bolt/lib/Core/BinaryFunction.cpp        | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index b21312f92c485..3c641581e247a 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -930,8 +930,7 @@ class BinaryFunction {
     return const_cast<BinaryFunction *>(this)->getInstructionAtOffset(Offset);
   }
 
-  const std::optional<MCInst>
-  disassembleInstructionAtOffset(uint64_t Offset) const;
+  std::optional<MCInst> disassembleInstructionAtOffset(uint64_t Offset) const;
 
   /// Return offset for the first instruction. If there is data at the
   /// beginning of a function then offset of the first instruction could
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 5f3c0cb1ad754..fb81fc3f2ba7b 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1167,7 +1167,7 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
   }
 }
 
-const std::optional<MCInst>
+std::optional<MCInst>
 BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
   assert(CurrentState == State::Empty);
   assert(Offset < MaxSize && "invalid offset");

>From 63725510bf85c9e3862800830f5881099ab4b21f Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 8 May 2024 11:59:59 -0700
Subject: [PATCH 5/6] Assert messages

Created using spr 1.3.4
---
 bolt/lib/Core/BinaryFunction.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index fb81fc3f2ba7b..4721a247ee2e2 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1169,10 +1169,10 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction,
 
 std::optional<MCInst>
 BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
-  assert(CurrentState == State::Empty);
-  assert(Offset < MaxSize && "invalid offset");
+  assert(CurrentState == State::Empty && "Function should not be disassembled");
+  assert(Offset < MaxSize && "Invalid offset");
   ErrorOr<ArrayRef<unsigned char>> FunctionData = getData();
-  assert(FunctionData && "cannot get function as data");
+  assert(FunctionData && "Cannot get function as data");
   MCInst Instr;
   uint64_t InstrSize = 0;
   const uint64_t InstrAddress = getAddress() + Offset;

>From 6ec0d9b1838bb5ba5c1fbe5730736c057f186493 Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 8 May 2024 12:02:06 -0700
Subject: [PATCH 6/6] clang-format

Created using spr 1.3.4
---
 bolt/lib/Core/BinaryFunction.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 4721a247ee2e2..de34421ebeb08 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1177,7 +1177,7 @@ BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const {
   uint64_t InstrSize = 0;
   const uint64_t InstrAddress = getAddress() + Offset;
   if (BC.DisAsm->getInstruction(Instr, InstrSize, FunctionData->slice(Offset),
-                                 InstrAddress, nulls()))
+                                InstrAddress, nulls()))
     return Instr;
   return std::nullopt;
 }



More information about the llvm-commits mailing list