[llvm] [BOLT] Fix preserved offset in fixDoubleJumps (PR #92485)

Amir Ayupov via llvm-commits llvm-commits at lists.llvm.org
Thu May 16 19:06:42 PDT 2024


https://github.com/aaupov created https://github.com/llvm/llvm-project/pull/92485

The offset was inherited from the containing basic block rather than
from branch instruction.

Test Plan: updated bolt/test/X86/bb-with-two-tail-calls.s

>From 6bfa8fbb1d33c458a8cb55242d1032d308aae965 Mon Sep 17 00:00:00 2001
From: Amir Ayupov <aaupov at fb.com>
Date: Wed, 15 May 2024 09:25:19 -0700
Subject: [PATCH] [BOLT] Fix preserved offset in fixDoubleJumps

The offset was inherited from the containing basic block rather than
from branch instruction.

Test Plan: updated bolt/test/X86/bb-with-two-tail-calls.s
---
 bolt/lib/Passes/BinaryPasses.cpp       | 14 +++++++++-----
 bolt/test/X86/bb-with-two-tail-calls.s |  8 ++++----
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp
index 867f977cebca7..298ba29ff5b3f 100644
--- a/bolt/lib/Passes/BinaryPasses.cpp
+++ b/bolt/lib/Passes/BinaryPasses.cpp
@@ -674,7 +674,8 @@ static uint64_t fixDoubleJumps(BinaryFunction &Function, bool MarkInvalid) {
   MCPlusBuilder *MIB = Function.getBinaryContext().MIB.get();
   for (BinaryBasicBlock &BB : Function) {
     auto checkAndPatch = [&](BinaryBasicBlock *Pred, BinaryBasicBlock *Succ,
-                             const MCSymbol *SuccSym) {
+                             const MCSymbol *SuccSym,
+                             std::optional<uint32_t> Offset) {
       // Ignore infinite loop jumps or fallthrough tail jumps.
       if (Pred == Succ || Succ == &BB)
         return false;
@@ -715,9 +716,11 @@ static uint64_t fixDoubleJumps(BinaryFunction &Function, bool MarkInvalid) {
           Pred->removeSuccessor(&BB);
           Pred->eraseInstruction(Pred->findInstruction(Branch));
           Pred->addTailCallInstruction(SuccSym);
-          MCInst *TailCall = Pred->getLastNonPseudoInstr();
-          assert(TailCall);
-          MIB->setOffset(*TailCall, BB.getOffset());
+          if (Offset) {
+            MCInst *TailCall = Pred->getLastNonPseudoInstr();
+            assert(TailCall);
+            MIB->setOffset(*TailCall, *Offset);
+          }
         } else {
           return false;
         }
@@ -760,7 +763,8 @@ static uint64_t fixDoubleJumps(BinaryFunction &Function, bool MarkInvalid) {
       if (Pred->getSuccessor() == &BB ||
           (Pred->getConditionalSuccessor(true) == &BB && !IsTailCall) ||
           Pred->getConditionalSuccessor(false) == &BB)
-        if (checkAndPatch(Pred, Succ, SuccSym) && MarkInvalid)
+        if (checkAndPatch(Pred, Succ, SuccSym, MIB->getOffset(*Inst)) &&
+            MarkInvalid)
           BB.markValid(BB.pred_size() != 0 || BB.isLandingPad() ||
                        BB.isEntryPoint());
     }
diff --git a/bolt/test/X86/bb-with-two-tail-calls.s b/bolt/test/X86/bb-with-two-tail-calls.s
index bb2b0cd4cc23a..b6703e352ff4b 100644
--- a/bolt/test/X86/bb-with-two-tail-calls.s
+++ b/bolt/test/X86/bb-with-two-tail-calls.s
@@ -1,8 +1,6 @@
 # This reproduces a bug with dynostats when trying to compute branch stats
 # at a block with two tails calls (one conditional and one unconditional).
 
-# REQUIRES: system-linux
-
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown \
 # RUN:   %s -o %t.o
 # RUN: link_fdata %s %t.o %t.fdata
@@ -13,7 +11,7 @@
 # CHECK-NOT: Assertion `BranchInfo.size() == 2 && "could only be called for blocks with 2 successors"' failed.
 # Two tail calls in the same basic block after SCTC:
 # CHECK:         {{.*}}:   ja      {{.*}} # TAILCALL # Offset: 7 # CTCTakenCount: 4
-# CHECK-NEXT:    {{.*}}:   jmp     {{.*}} # TAILCALL # Offset: 12
+# CHECK-NEXT:    {{.*}}:   jmp     {{.*}} # TAILCALL # Offset: 13
 
   .globl _start
 _start:
@@ -23,7 +21,9 @@ a:  ja b
 x:  ret
 # FDATA: 1 _start #a# 1 _start #b# 2 4
 b:  jmp e
-c:  jmp f
+c:
+    .nops 1
+    jmp f
 
   .globl e
 e:



More information about the llvm-commits mailing list