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

[BOLT][NFC] Rename isUnsupportedBranch to isReversibleBranch #92447

Merged
merged 1 commit into from
May 17, 2024

Conversation

urnathan
Copy link
Contributor

isUnsupportedBranch is not a very informative name, and doesn't match its corresponding reverseBranchCondition, as I noted in PR #92018. Here's a renaming to a more mnemonic name.

@urnathan urnathan marked this pull request as ready for review May 17, 2024 10:55
@llvmbot llvmbot added the BOLT label May 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 17, 2024

@llvm/pr-subscribers-bolt

Author: Nathan Sidwell (urnathan)

Changes

isUnsupportedBranch is not a very informative name, and doesn't match its corresponding reverseBranchCondition, as I noted in PR #92018. Here's a renaming to a more mnemonic name.


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

4 Files Affected:

  • (modified) bolt/include/bolt/Core/MCPlusBuilder.h (+2-2)
  • (modified) bolt/lib/Core/BinaryFunction.cpp (+2-2)
  • (modified) bolt/lib/Passes/Instrumentation.cpp (+1-1)
  • (modified) bolt/lib/Target/X86/X86MCPlusBuilder.cpp (+5-5)
diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h
index f7614cf9ac977..f7cf538bd0e86 100644
--- a/bolt/include/bolt/Core/MCPlusBuilder.h
+++ b/bolt/include/bolt/Core/MCPlusBuilder.h
@@ -438,8 +438,8 @@ class MCPlusBuilder {
     return false;
   }
 
-  /// Check whether we support inverting this branch
-  virtual bool isUnsupportedBranch(const MCInst &Inst) const { return false; }
+  /// Check whether this conditional branch can be reversed
+  virtual bool isReversibleBranch(const MCInst &Inst) const { return true; }
 
   /// 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 de34421ebeb08..a9c588fc6e060 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1284,7 +1284,7 @@ Error BinaryFunction::disassemble() {
         const bool IsCondBranch = MIB->isConditionalBranch(Instruction);
         MCSymbol *TargetSymbol = nullptr;
 
-        if (BC.MIB->isUnsupportedBranch(Instruction)) {
+        if (!BC.MIB->isReversibleBranch(Instruction)) {
           setIgnored();
           if (BinaryFunction *TargetFunc =
                   BC.getBinaryFunctionContainingAddress(TargetAddress))
@@ -3384,7 +3384,7 @@ void BinaryFunction::fixBranches() {
 
       // Reverse branch condition and swap successors.
       auto swapSuccessors = [&]() {
-        if (MIB->isUnsupportedBranch(*CondBranch)) {
+        if (!MIB->isReversibleBranch(*CondBranch)) {
           if (opts::Verbosity) {
             BC.outs() << "BOLT-INFO: unable to swap successors in " << *this
                       << '\n';
diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index 68acff7e6a867..14f506f9ca968 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -480,7 +480,7 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
       else if (BC.MIB->isUnconditionalBranch(Inst))
         HasUnconditionalBranch = true;
       else if ((!BC.MIB->isCall(Inst) && !BC.MIB->isConditionalBranch(Inst)) ||
-               BC.MIB->isUnsupportedBranch(Inst))
+               !BC.MIB->isReversibleBranch(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 8b1894953f375..8fdacffcb147b 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 isUnsupportedBranch(const MCInst &Inst) const override {
+  bool isReversibleBranch(const MCInst &Inst) const override {
     if (isDynamicBranch(Inst))
-      return true;
+      return false;
 
     switch (Inst.getOpcode()) {
     default:
-      return false;
+      return true;
     case X86::LOOP:
     case X86::LOOPE:
     case X86::LOOPNE:
     case X86::JECXZ:
     case X86::JRCXZ:
-      return true;
+      return false;
     }
   }
 
@@ -1874,7 +1874,7 @@ class X86MCPlusBuilder : public MCPlusBuilder {
       }
 
       // Handle conditional branches and ignore indirect branches
-      if (!isUnsupportedBranch(*I) && getCondCode(*I) == X86::COND_INVALID) {
+      if (isReversibleBranch(*I) && getCondCode(*I) == X86::COND_INVALID) {
         // Indirect branch
         return false;
       }

Copy link
Contributor

@aaupov aaupov left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks!

@urnathan urnathan merged commit 76fdc2e into llvm:main May 17, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants