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 d13e28999a05c..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,12 +1292,10 @@ Error BinaryFunction::disassemble() { const bool IsCondBranch = MIB->isConditionalBranch(Instruction); MCSymbol *TargetSymbol = nullptr; - if (!BC.MIB->isReversibleBranch(Instruction)) { - setIgnored(); - if (BinaryFunction *TargetFunc = + if (IsUnsupported) + if (auto *TargetFunc = BC.getBinaryFunctionContainingAddress(TargetAddress)) TargetFunc->setIgnored(); - } if (IsCall && containsAddress(TargetAddress)) { if (TargetAddress == getAddress()) { 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);