[llvm] 748d92b - Simplify MachineVerifier's block-successor verification.
James Y Knight via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 6 19:41:49 PDT 2020
Author: James Y Knight
Date: 2020-06-06T22:30:51-04:00
New Revision: 748d92b4d3142ad57266211351d3377ed976f3dc
URL: https://github.com/llvm/llvm-project/commit/748d92b4d3142ad57266211351d3377ed976f3dc
DIFF: https://github.com/llvm/llvm-project/commit/748d92b4d3142ad57266211351d3377ed976f3dc.diff
LOG: Simplify MachineVerifier's block-successor verification.
There's two properties we want to verify:
1. That the successors returned by analyzeBranch are in the CFG
successor list, and
2. That there are no extraneous successors are in the CFG successor
list.
The previous implementation mostly accomplished this, but in a very
convoluted manner.
Differential Revision: https://reviews.llvm.org/D79793
Added:
Modified:
llvm/lib/CodeGen/MachineVerifier.cpp
llvm/test/CodeGen/Hexagon/cext-opt-range-offset.mir
llvm/test/CodeGen/WebAssembly/eh-labels.mir
llvm/test/MachineVerifier/verifier-pseudo-terminators.mir
Removed:
################################################################################
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index b6121c79aad2..d0568b35505e 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -573,16 +573,6 @@ void MachineVerifier::visitMachineFunctionBefore() {
verifyStackFrame();
}
-// Does iterator point to a and b as the first two elements?
-static bool matchPair(MachineBasicBlock::const_succ_iterator i,
- const MachineBasicBlock *a, const MachineBasicBlock *b) {
- if (*i == a)
- return *++i == b;
- if (*i == b)
- return *++i == a;
- return false;
-}
-
void
MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
FirstTerminator = nullptr;
@@ -616,20 +606,6 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
}
}
- // Count the number of INLINEASM_BR indirect target successors.
- SmallPtrSet<const MachineBasicBlock*, 4> IndirectTargetSuccs;
- for (const auto *succ : MBB->successors()) {
- if (MBB->isInlineAsmBrIndirectTarget(succ))
- IndirectTargetSuccs.insert(succ);
- if (!FunctionBlocks.count(succ))
- report("MBB has successor that isn't part of the function.", MBB);
- if (!MBBInfoMap[succ].Preds.count(MBB)) {
- report("Inconsistent CFG", MBB);
- errs() << "MBB is not in the predecessor list of the successor "
- << printMBBReference(*succ) << ".\n";
- }
- }
-
// Check the predecessor list.
for (const MachineBasicBlock *Pred : MBB->predecessors()) {
if (!FunctionBlocks.count(Pred))
@@ -660,26 +636,6 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
// check whether its answers match up with reality.
if (!TBB && !FBB) {
// Block falls through to its successor.
- MachineFunction::const_iterator MBBI = std::next(MBB->getIterator());
- if (MBBI == MF->end()) {
- // It's possible that the block legitimately ends with a noreturn
- // call or an unreachable, in which case it won't actually fall
- // out the bottom of the function.
- } else if (MBB->succ_size() == LandingPadSuccs.size() ||
- MBB->succ_size() == IndirectTargetSuccs.size()) {
- // It's possible that the block legitimately ends with a noreturn
- // call or an unreachable, in which case it won't actually fall
- // out of the block.
- } else if ((LandingPadSuccs.size() &&
- MBB->succ_size() != 1 + LandingPadSuccs.size()) ||
- (IndirectTargetSuccs.size() &&
- MBB->succ_size() != 1 + IndirectTargetSuccs.size())) {
- report("MBB exits via unconditional fall-through but doesn't have "
- "exactly one CFG successor!", MBB);
- } else if (!MBB->isSuccessor(&*MBBI)) {
- report("MBB exits via unconditional fall-through but its successor "
- "
diff ers from its CFG successor!", MBB);
- }
if (!MBB->empty() && MBB->back().isBarrier() &&
!TII->isPredicated(MBB->back())) {
report("MBB exits via unconditional fall-through but ends with a "
@@ -691,20 +647,6 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
}
} else if (TBB && !FBB && Cond.empty()) {
// Block unconditionally branches somewhere.
- // If the block has exactly one successor, that happens to be a
- // landingpad, accept it as valid control flow.
- if (MBB->succ_size() != 1+LandingPadSuccs.size() &&
- (MBB->succ_size() != 1 || LandingPadSuccs.size() != 1 ||
- *MBB->succ_begin() != *LandingPadSuccs.begin()) &&
- MBB->succ_size() != 1 + IndirectTargetSuccs.size() &&
- (MBB->succ_size() != 1 || IndirectTargetSuccs.size() != 1 ||
- *MBB->succ_begin() != *IndirectTargetSuccs.begin())) {
- report("MBB exits via unconditional branch but doesn't have "
- "exactly one CFG successor!", MBB);
- } else if (!MBB->isSuccessor(TBB)) {
- report("MBB exits via unconditional branch but the CFG "
- "successor doesn't match the actual successor!", MBB);
- }
if (MBB->empty()) {
report("MBB exits via unconditional branch but doesn't contain "
"any instructions!", MBB);
@@ -717,24 +659,6 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
}
} else if (TBB && !FBB && !Cond.empty()) {
// Block conditionally branches somewhere, otherwise falls through.
- MachineFunction::const_iterator MBBI = std::next(MBB->getIterator());
- if (MBBI == MF->end()) {
- report("MBB conditionally falls through out of function!", MBB);
- } else if (MBB->succ_size() == 1) {
- // A conditional branch with only one successor is weird, but allowed.
- if (&*MBBI != TBB)
- report("MBB exits via conditional branch/fall-through but only has "
- "one CFG successor!", MBB);
- else if (TBB != *MBB->succ_begin())
- report("MBB exits via conditional branch/fall-through but the CFG "
- "successor don't match the actual successor!", MBB);
- } else if (MBB->succ_size() != 2) {
- report("MBB exits via conditional branch/fall-through but doesn't have "
- "exactly two CFG successors!", MBB);
- } else if (!matchPair(MBB->succ_begin(), TBB, &*MBBI)) {
- report("MBB exits via conditional branch/fall-through but the CFG "
- "successors don't match the actual successors!", MBB);
- }
if (MBB->empty()) {
report("MBB exits via conditional branch/fall-through but doesn't "
"contain any instructions!", MBB);
@@ -748,21 +672,6 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
} else if (TBB && FBB) {
// Block conditionally branches somewhere, otherwise branches
// somewhere else.
- if (MBB->succ_size() == 1) {
- // A conditional branch with only one successor is weird, but allowed.
- if (FBB != TBB)
- report("MBB exits via conditional branch/branch through but only has "
- "one CFG successor!", MBB);
- else if (TBB != *MBB->succ_begin())
- report("MBB exits via conditional branch/branch through but the CFG "
- "successor don't match the actual successor!", MBB);
- } else if (MBB->succ_size() != 2) {
- report("MBB exits via conditional branch/branch but doesn't have "
- "exactly two CFG successors!", MBB);
- } else if (!matchPair(MBB->succ_begin(), TBB, FBB)) {
- report("MBB exits via conditional branch/branch but the CFG "
- "successors don't match the actual successors!", MBB);
- }
if (MBB->empty()) {
report("MBB exits via conditional branch/branch but doesn't "
"contain any instructions!", MBB);
@@ -780,6 +689,53 @@ MachineVerifier::visitMachineBasicBlockBefore(const MachineBasicBlock *MBB) {
} else {
report("analyzeBranch returned invalid data!", MBB);
}
+
+ // Now check that the successors match up with the answers reported by
+ // analyzeBranch.
+ if (TBB && !MBB->isSuccessor(TBB))
+ report("MBB exits via jump or conditional branch, but its target isn't a "
+ "CFG successor!",
+ MBB);
+ if (FBB && !MBB->isSuccessor(FBB))
+ report("MBB exits via conditional branch, but its target isn't a CFG "
+ "successor!",
+ MBB);
+
+ // There might be a fallthrough to the next block if there's either no
+ // unconditional true branch, or if there's a condition, and one of the
+ // branches is missing.
+ bool Fallthrough = !TBB || (!Cond.empty() && !FBB);
+
+ // A conditional fallthrough must be an actual CFG successor, not
+ // unreachable. (Conversely, an unconditional fallthrough might not really
+ // be a successor, because the block might end in unreachable.)
+ if (!Cond.empty() && !FBB) {
+ MachineFunction::const_iterator MBBI = std::next(MBB->getIterator());
+ if (MBBI == MF->end()) {
+ report("MBB conditionally falls through out of function!", MBB);
+ } else if (!MBB->isSuccessor(&*MBBI))
+ report("MBB exits via conditional branch/fall-through but the CFG "
+ "successors don't match the actual successors!",
+ MBB);
+ }
+
+ // Verify that there aren't any extra un-accounted-for successors.
+ for (const MachineBasicBlock *SuccMBB : MBB->successors()) {
+ // If this successor is one of the branch targets, it's okay.
+ if (SuccMBB == TBB || SuccMBB == FBB)
+ continue;
+ // If we might have a fallthrough, and the successor is the fallthrough
+ // block, that's also ok.
+ if (Fallthrough && SuccMBB == MBB->getNextNode())
+ continue;
+ // Also accept successors which are for exception-handling or might be
+ // inlineasm_br targets.
+ if (SuccMBB->isEHPad() || MBB->isInlineAsmBrIndirectTarget(SuccMBB))
+ continue;
+ report("MBB has unexpected successors which are not branch targets, "
+ "fallthrough, EHPads, or inlineasm_br targets.",
+ MBB);
+ }
}
regsLive.clear();
diff --git a/llvm/test/CodeGen/Hexagon/cext-opt-range-offset.mir b/llvm/test/CodeGen/Hexagon/cext-opt-range-offset.mir
index ea19e6be1aa6..9a012e830af8 100644
--- a/llvm/test/CodeGen/Hexagon/cext-opt-range-offset.mir
+++ b/llvm/test/CodeGen/Hexagon/cext-opt-range-offset.mir
@@ -36,7 +36,6 @@ body: |
successors: %bb.4
bb.4:
- successors: %bb.4
%5 = A2_tfrsi -234944521
%6 = A2_tfrsi -360185632
L4_and_memopw_io %6, 0, %5
diff --git a/llvm/test/CodeGen/WebAssembly/eh-labels.mir b/llvm/test/CodeGen/WebAssembly/eh-labels.mir
index 9dac3e8c67c0..673d34869b73 100644
--- a/llvm/test/CodeGen/WebAssembly/eh-labels.mir
+++ b/llvm/test/CodeGen/WebAssembly/eh-labels.mir
@@ -29,6 +29,7 @@ body: |
EH_LABEL <mcsymbol .Ltmp0>
CALL @foo, implicit-def dead $arguments, implicit $sp32, implicit $sp64
EH_LABEL <mcsymbol .Ltmp1>
+ BR %bb.2, implicit-def dead $arguments
bb.1 (landing-pad):
; predecessors: %bb.0
diff --git a/llvm/test/MachineVerifier/verifier-pseudo-terminators.mir b/llvm/test/MachineVerifier/verifier-pseudo-terminators.mir
index 6e9ec9c0887f..c289742e8caf 100644
--- a/llvm/test/MachineVerifier/verifier-pseudo-terminators.mir
+++ b/llvm/test/MachineVerifier/verifier-pseudo-terminators.mir
@@ -4,7 +4,8 @@
# Make sure that mismatched successors are caught when a _term
# instruction is used
-# CHECK: *** Bad machine code: MBB exits via unconditional branch but the CFG successor doesn't match the actual successor! ***
+# CHECK: *** Bad machine code: MBB exits via jump or conditional branch, but its target isn't a CFG successor! ***
+# CHECK: *** Bad machine code: MBB has unexpected successors which are not branch targets, fallthrough, EHPads, or inlineasm_br targets. ***
---
name: verifier_pseudo_terminators
More information about the llvm-commits
mailing list