[PATCH] D76961: [SelectionDAG] fix predecessor list for INLINEASM_BRs' parent

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2950
+  // PHI node fixup on First that SelectionDAGISel::FinishBasicBlock will
+  // perform on Last.
+  if (First->getFirstTerminator()->getOpcode() == TargetOpcode::INLINEASM_BR)
----------------
efriedma wrote:
> Maybe it would be better to move this into SelectionDAGISel::FinishBasicBlock?  All the other similar code seems to be there.
> 
> If we do keep it here, better to fix the comment to something like "SelectionDAGISel::FinishBasicBlock will add PHI operands for the successors of the fallthrough block.  Here, we add PHI operands for the successors of the INLINEASM_BR block itself".  (It's not obvious at first glance what "first" and "last" refer to.)
The relevant call chain look like:

```
SelectionDAGISel::SelectAllBasicBlocks
  SelectBasicBlock
    CodeGenAndEmitDAG
      ScheduleDAGSDNodes::EmitSchedule
      SelectionDAGBuilder::UpdateSplitBlock
  FinishBasicBlock
```
The issue is that `ScheduleDAGSDNodes::EmitSchedule` splits `FuncInfo->MBB` (the current `MachineBasicBlock` we're emitting a schedule for), then the return value is used to update `FuncInfo->MBB` in `CodeGenAndEmitDAG`, such that later in `FinishBasicBlock` we no longer have a reference to the block referred to as `First` in `SelectionDAGBuilder::UpdateSplitBlock`.  `SelectionDAGBuilder::UpdateSplitBlock` is only called when `FuncInfo->MBB` is split via call to `ScheduleDAGSDNodes::EmitSchedule`, so I'm certain it make sense to perform special clean up there.  In fact, the comment in `SelectionDAGISel::CodeGenAndEmitDAG` says:
```
  // If the block was split, make sure we update any references that are used to
  // update PHI nodes later on.
  if (FirstMBB != LastMBB)
    SDB->UpdateSplitBlock(FirstMBB, LastMBB);
```
so again it makes sense for us to update the PHI nodes given a block split in `UpdateSplitBlock`.

I've updated the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76961