[PATCH] D76961: [BranchFolder] don't remove MBB's that have their address taken

[Nick Desaulniers via Phabricator via llvm-commits]

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