[llvm] [BOLT] Do not reject irreversible branches during disassembly (PR #95572)

[Nathan Sidwell via llvm-commits]

https://github.com/urnathan updated https://github.com/llvm/llvm-project/pull/95572

>From 17f179d009560d7c6d86cc93c03e0deccc681484 Mon Sep 17 00:00:00 2001
From: Nathan Sidwell <nathan at acm.org>
Date: Fri, 14 Jun 2024 13:00:20 -0400
Subject: [PATCH 1/2] [BOLT] Do not reject irreversible branches during
 disassembly

Now that `isUnsupportedBranch` is renamed (and inverted) to
`isReversibleBranch`, this use in `BinaryFunction::disassemble` sticks
out like a sore thumb. My guess is that at one point one needed to
reject unintelligible branches early, and that morphed into the use
for detecting uninvertible branches.

Removing this check results in no new test failures.

The use of `isReversibleBranch` here raises questions about what it
should return for instructions that are not conditional branches --
given the other uses of this function I had been presuming 'don't
care'.

If some branches should be rejected at this point, a new MCPlusBuilder
hook is most likely needed -- this cannot be the correct predicate to
use (after all, it would render all the other uses moot.
---
 bolt/lib/Core/BinaryFunction.cpp | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index d13e28999a05c..a8b1f69166d21 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1288,13 +1288,6 @@ Error BinaryFunction::disassemble() {
         const bool IsCondBranch = MIB->isConditionalBranch(Instruction);
         MCSymbol *TargetSymbol = nullptr;
 
-        if (!BC.MIB->isReversibleBranch(Instruction)) {
-          setIgnored();
-          if (BinaryFunction *TargetFunc =
-                  BC.getBinaryFunctionContainingAddress(TargetAddress))
-            TargetFunc->setIgnored();
-        }
-
         if (IsCall && containsAddress(TargetAddress)) {
           if (TargetAddress == getAddress()) {
             // Recursive call.

>From 0408a96e6a6cc7a19cdc70e298d24c572e09f1cb Mon Sep 17 00:00:00 2001
From: Nathan Sidwell <nathan at acm.org>
Date: Fri, 21 Jun 2024 14:54:13 -0400
Subject: [PATCH 2/2] Add new isUnsupportedInst & adjust isReversibleBranch

---
 bolt/include/bolt/Core/MCPlusBuilder.h   | 15 ++++++++++++++-
 bolt/lib/Core/BinaryFunction.cpp         |  9 +++++++++
 bolt/lib/Passes/Instrumentation.cpp      |  3 +--
 bolt/lib/Target/X86/X86MCPlusBuilder.cpp | 18 ++++++++----------
 4 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index 765372aa9e402..83f4d4c649fd8 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -439,7 +439,20 @@ class MCPlusBuilder {
   }
 
   /// Check whether this conditional branch can be reversed
-  virtual bool isReversibleBranch(const MCInst &Inst) const { return true; }
+  virtual bool isReversibleBranch(const MCInst &Inst) const {
+    assert(!isUnsupportedInstruction(Inst) && isConditionalBranch(Inst) &&
+           "Instruction is not known conditional branch");
+
+    if (isDynamicBranch(Inst))
+      return false;
+    return true;
+  }
+
+  /// Return true if this instruction inhibits analysis of the containing
+  /// function.
+  virtual bool isUnsupportedInstruction(const MCInst &Inst) const {
+    return false;
+  }
 
   /// Return true of the instruction is of pseudo kind.
   virtual bool isPseudo(const MCInst &Inst) const {
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index a8b1f69166d21..d2ca0fcdf6b7a 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1275,6 +1275,10 @@ Error BinaryFunction::disassemble() {
       }
     }
 
+    bool IsUnsupported = BC.MIB->isUnsupportedInstruction(Instruction);
+    if (IsUnsupported)
+      setIgnored();
+
     if (MIB->isBranch(Instruction) || MIB->isCall(Instruction)) {
       uint64_t TargetAddress = 0;
       if (MIB->evaluateBranch(Instruction, AbsoluteInstrAddr, Size,
@@ -1288,6 +1292,11 @@ Error BinaryFunction::disassemble() {
         const bool IsCondBranch = MIB->isConditionalBranch(Instruction);
         MCSymbol *TargetSymbol = nullptr;
 
+        if (IsUnsupported)
+          if (auto *TargetFunc =
+                  BC.getBinaryFunctionContainingAddress(TargetAddress))
+            TargetFunc->setIgnored();
+
         if (IsCall && containsAddress(TargetAddress)) {
           if (TargetAddress == getAddress()) {
             // Recursive call.
diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index 14f506f9ca968..e824a42d82696 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -479,8 +479,7 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
         HasJumpTable = true;
       else if (BC.MIB->isUnconditionalBranch(Inst))
         HasUnconditionalBranch = true;
-      else if ((!BC.MIB->isCall(Inst) && !BC.MIB->isConditionalBranch(Inst)) ||
-               !BC.MIB->isReversibleBranch(Inst))
+      else if (!(BC.MIB->isCall(Inst) || BC.MIB->isConditionalBranch(Inst)))
         continue;
 
       const uint32_t FromOffset = *BC.MIB->getOffset(Inst);
diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
index 515c9a94c58cd..5bd77958934f9 100644
--- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
+++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp
@@ -328,19 +328,19 @@ class X86MCPlusBuilder : public MCPlusBuilder {
     return false;
   }
 
-  bool isReversibleBranch(const MCInst &Inst) const override {
-    if (isDynamicBranch(Inst))
-      return false;
-
+  bool isUnsupportedInstruction(const MCInst &Inst) const override {
     switch (Inst.getOpcode()) {
     default:
-      return true;
+      return false;
+
     case X86::LOOP:
     case X86::LOOPE:
     case X86::LOOPNE:
     case X86::JECXZ:
     case X86::JRCXZ:
-      return false;
+      // These have a short displacement, and therefore (often) break after
+      // basic block relayout.
+      return true;
     }
   }
 
@@ -1879,11 +1879,9 @@ class X86MCPlusBuilder : public MCPlusBuilder {
         continue;
       }
 
-      // Handle conditional branches and ignore indirect branches
-      if (isReversibleBranch(*I) && getCondCode(*I) == X86::COND_INVALID) {
-        // Indirect branch
+      // Ignore indirect branches
+      if (getCondCode(*I) == X86::COND_INVALID)
         return false;
-      }
 
       if (CondBranch == nullptr) {
         const MCSymbol *TargetBB = getTargetSymbol(*I);