[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#1951329 <https://reviews.llvm.org/D76961#1951329>, @nickdesaulniers wrote:

> In D76961#1951241 <https://reviews.llvm.org/D76961#1951241>, @efriedma wrote:
>
> > > Probably in InstrEmitter::EmitSpecialNode.
> >
> > We normally construct the MachineFunction CFG much earlier; see SelectionDAGBuilder::visitCallBr.
> >
> > It looks like the problem is that we split the "block" into two MBBs, and the successors end up attached to the second block, instead of the first.  Not sure where that happens, off the top of my head.
>
>
> I think this is what's going on in `ScheduleDAGSDNodes::EmitSchedule`; the results of the before/after split don't look quite right in terms of pred/succ lists, but I've got to dig more into this tomorrow.


Yeah, this is what's going on. Printing the `MachineBasicBlocks` before and after the relevant `INLINEASM_BR` part of `ScheduleDAGSDNodes::EmitSchedule` shows:

Before:

  bb.0 (%ir-block.2):
    successors: %bb.1(0x80000000), %bb.4(0x00000000); %bb.1(100.00%), %bb.4(0.00%)
    INLINEASM_BR &"jmp ${1:l}" [attdialect], $0:[regdef:GR32], def %5:gr32, $1:[imm], blockaddress(@main, %ir-block.11), $2:[clobber], implicit-def early-clobber $df, $3:[clobber], implicit-def early-clobber $fpsw, $4:[clobber], implicit-def early-clobber $eflags, !2
    %0:gr32 = COPY %5:gr32
    JMP_1 %bb.1
  
  bb.1 (%ir-block.4):
  ; predecessors: %bb.0
  ...
  bb.4 (%ir-block.11, address-taken):
  ; predecessors: %bb.0

Everything looks good.  I don't understand why `INLINEASM_BR` isn't the terminal instruction of `bb.0`.  Isn't it marked a terminator in `llvm/include/llvm/Target/Target.td` line 1021?  I also also don't understand why the `MachineBasicBlock` has a `terminators` method plural?  I thought all blocks have 1 and only 1 terminal instruction?

Afterwards, things look bad:

  bb.0 (%ir-block.2):
    successors: %bb.6(0x80000000); %bb.6(100.00%)
  
    INLINEASM_BR &"jmp ${1:l}" [attdialect], $0:[regdef:GR32], def %5:gr32, $1:[imm], blockaddress(@main, %ir-block.11), $2:[clobber], implicit-def early-clobber $df, $3:[clobber], implicit-def early-clobber $fpsw, $4:[clobber], implicit-def early-clobber $eflags, !2
  
  bb.6 (%ir-block.2):
  ; predecessors: %bb.0
    successors: %bb.1(0x80000000), %bb.4(0x00000000); %bb.1(100.00%), %bb.4(0.00%)
  
    %0:gr32 = COPY %5:gr32
    JMP_1 %bb.1
  
  bb.1 (%ir-block.4):
  ; predecessors: %bb.6
  ...
  bb.4 (%ir-block.11, address-taken):
  ; predecessors: %bb.6

Specifically:

1. `bb.0` has one successor, `bb.6`, which isn't right as the `INLINEASM_BR` could jump to `bb.4`.  `bb.0` should have two successors (fallthrough: `bb.6`, indirect target: `bb.4`)
2. `bb.6` (the `CopyBB` in `ScheduleDAGSDNodes::EmitSchedule` successor list is wrong; `bb.1` is right, but `bb.6` cannot get to `bb.4` as it unconditionally jumps to `bb.1` only.

When we split the initial `MachineBasicBlock`, we need to "take" only the fallthrough successor, I suspect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76961