[PATCH] D76961: [BranchFolder] don't remove MBB's that have their address taken
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 30 15:17:09 PDT 2020
nickdesaulniers added a comment.
In D76961#1950390 <https://reviews.llvm.org/D76961#1950390>, @nickdesaulniers wrote:
> 2. all `MachineBasicBlocks` that are targets of `INLINEASM_BR` should have the `INLINEASM_BR`'s parent `MachineBasicBlock` in their list of predacessors. This is the invariant that's violated in the above test case.
In D76961#1950917 <https://reviews.llvm.org/D76961#1950917>, @nickdesaulniers wrote:
> This fails on our test suite in `llvm/test/CodeGen/X86/callbr-asm-outputs.ll`'s first test case. Right off the bat, I can see we messed this up in ISEL:
>
> # *** IR Dump After Finalize ISel and expand pseudo-instructions ***:
> ...
> bb.0.entry:
> successors: %bb.3(0x80000000); %bb.3(100.00%)
> ...
> INLINEASM_BR &"xorl $1, $0; jmp ${2:l}" [attdialect], $0:[regdef:GR32], def %2:gr32, $1:[reguse:GR32], %3:gr32, $2:[imm], blockaddress(@test1, %ir-block.abnormal), $3:[clobber], implicit-def early-clobber $df, $4:[clobber], implicit-def early-clobber $fpsw, $5:[clobber], implicit-def early-clobber $eflags
> bb.3.entry:
> ; predecessors: %bb.0
> ...
> bb.2.abnormal (address-taken):
> ; predecessors: %bb.3
> ...
>
We can see the second invariant already violated post ISEL here as well. FWIW, a check might look like:
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 72da33c92346..871c61e76326 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -887,6 +887,28 @@ void MachineVerifier::verifyInlineAsm(const MachineInstr *MI) {
if (!MO.isReg() || !MO.isImplicit())
report("Expected implicit register after groups", &MO, OpNo);
}
+
+ if (MI->getOpcode() == TargetOpcode::INLINEASM_BR) {
+ for (const MachineOperand& MO : MI->operands())
+ if (MO.isBlockAddress()) {
+ const BasicBlock* BB = MO.getBlockAddress()->getBasicBlock();
+ const MachineBasicBlock* Target = nullptr;
+ for (const MachineBasicBlock& MBB : *MI->getParent()->getParent()) {
+ if (MBB.getBasicBlock() == BB)
+ Target = &MBB;
+ }
+ if (Target) {
+ SmallPtrSet<const MachineBasicBlock*, 2> Preds;
+ for (const MachineBasicBlock* Pred : Target->predecessors()) {
+ Preds.insert(Pred);
+ }
+ if (!Preds.count(MI->getParent()))
+ report("MMB target of INLINEASM_BR, but missing INLINEASM_BR's parent MBB from predecessor list", Target);
+ } else {
+ report("could not find target MBB", MI);
+ }
+ }
+ }
So I think we need to fix up ISEL to properly set successors and predecessors for `MachineBasicBlocks` that are terminated by `INLINEASM_BR` `MachineInstr`s. Does that sound right, @void @efriedma ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76961/new/
https://reviews.llvm.org/D76961
More information about the llvm-commits
mailing list