[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:

> 1. implement separate machine verifier checks for:
>   1. all `MachineBasicBlocks` with `INLINEASM_BR` terminators should have the targets of the `INLINEASM_BR` in their list of successors.


Ok, so I don't think this is generally the case, as we could have a `MachineOperand` to the `INLINEASM_BR` that is a `blockaddress`, yet is not a successor;  when inline asm passes the address of a label ("computed goto") as input to an asm statement, but not in the label list, that doesn't make a it valid branch target (though theoretically, the inline asm could still jump to it, which is super undefined).

But with a basic check:

  diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
  index 72da33c92346..94b1bd31a4e9 100644
  --- a/llvm/lib/CodeGen/MachineVerifier.cpp
  +++ b/llvm/lib/CodeGen/MachineVerifier.cpp
  @@ -887,6 +887,17 @@ 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) {
  +    // Check that the targets are listed as successors to this MI's parent MBB.
  +    SmallPtrSet<const BasicBlock*, 2> successors;
  +    for (const MachineBasicBlock* MBB : MI->getParent()->successors())
  +      successors.insert(MBB->getBasicBlock());
  +    for (const MachineOperand& MO : MI->operands())
  +      if (MO.isBlockAddress())
  +        if (!successors.count(MO.getBlockAddress()->getBasicBlock()))
  +          report("Expected INLINEASM_BR blockaddress operand not in successor list of parent MBB", MI);
  +  }
   }

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 we have an `INLINEASM_BR` `MachineInstr` with a `blockaddress` `MachineOperand`, but the parent `MachineBasicBlock` (`bb.0`) of the `INLINEASM_BR` `MachineInstr`'s successor list does not include the `MachineBasicBlock` for which the `blockaddress` `MachineOperand` refers (`bb.2`).

(Also, it's un-ergonomic that `blockaddress`es at the `MachineInstr` level seem to refer to `BasicBlock`s, and not `MachineBasicBlock`s.  You have to call `MachineBasicBlock#getBasicBlock()`.  I honestly don't like `blockaddress`es and wonder if we could make these all just have `BasicBlock`s or `MachineBasicBlock`s as operands?)

I'm going to check the other invariant I specified above now, and will report back on that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76961/new/

https://reviews.llvm.org/D76961