[PATCH] D79605: MachineBasicBlock::updateTerminator now requires an explicit layout successor.

[James Y Knight via Phabricator via llvm-commits]

jyknight marked 3 inline comments as done.
jyknight added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:526
+  /// may now be able to fallthrough to the current layout successor.
+  void updateTerminator(MachineBasicBlock *PreviousLayoutSuccessor);
 
----------------
aheejin wrote:
> I'm not very sure `PreviousLayoutSuccessor` stands for. `updateTerminator` does not change layout (= order of BBs), so the layout successor should stay the same before and after calling `updateTerminator`. Depending on branches created, after calling `updateTerminator`, we may not end up falling through to the `PreviousLayoutSuccessor` variable, but in that case it still succeeds MBB in layout. `PreviousLayoutSuccessor`, to me, sounds like the layout successor will change after calling this function. How about just `LayoutSuccessor` or something..?
In some (but not all) of the callers, the current actual block ordering of the function has been modified _before_ calling this function. Therefore, the argument passed as "PreviousLayoutSuccessor" is no longer the actual, current, layout successor, at the time of this call.

If there is currently a fallthrough at the end of the block, a branch may need to be inserted, and the target of that branch is PreviousLayoutSuccessor.



================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:586
+      // The block has an unconditional fallthrough (or the end of the block is
+      // unreachable).
 
----------------
efriedma wrote:
> efriedma wrote:
> > jyknight wrote:
> > > efriedma wrote:
> > > > "or the end of the block is unreachable" makes me a little worried.  If I'm understanding correctly, if do somehow end up in that situation, the layout successor won't actually be a successor of the BasicBlock, and `assert(isSuccessor(PreviousLayoutSuccessor))` will fail.
> > > Um, yup. I think you've identified a serious issue.
> > > 
> > > Identifying whether the instruction stream represents a fallthrough is another place where we look at successor list to help figure out what the instructions mean. That's something else I should fix -- we currently don't actually represent the tail of the block being unreachable in the instruction stream. IMO, this is a mistake, and we ought to have an UNREACHABLE_MARKER pseudo-inst to represent this situation, and stop depending on querying the successors list to figure this out.
> > > 
> > > For the purposes of this patch and making incremental progress, I believe it's sufficient to just delete the assert and go with `if(!PreviousLayoutSuccessor || !isSuccessor(PreviousLayoutSuccessor)) return;`. That won't fully achieve my goal, but I'll work on the above idea coupled with a cleanup of all the fallthrough handling, here and everywhere else, as a separate change.
> > Yes, I think that works.
> Hmm.  Actually, isSuccessor() isn't sufficient, I think.  Consider the case where the end of the block is unreachable, and the layout successor is an EHPad.
Agreed.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:626
-    if ((*SI)->isEHPad() || *SI == TBB)
-      continue;
-    assert(!FallthroughBB && "Found more than one fallthrough successor.");
----------------
aheejin wrote:
> efriedma wrote:
> > aheejin wrote:
> > > Does this new patch handle when a given layout successor is an EH pad? I might be mistaken, but I think that case should be excluded. But while this code is deleted, I can't find a similar counterpart in the new CL.
> > There are three possibilities here:
> > 
> > 1. The end of the block is unreachable.  This is case we discussed in the other comment.
> > 2. We have fallthrough.  The original layout successor can't be an EHPad because it's illegal to fall through into an EHPad.
> > 3. We don't have fallthrough.  In this case, the original layout successor is irrelevant.
> I'm not sure about case 2; the reason is here: https://reviews.llvm.org/D79605#2035699
The problem right now (discussed in the other comment) is that we cannot easily distinguish between actual fallthrough and unreachable block-end.

So, yes, right now we may end up in a situation that appears as though a fallthrough occurs into an EHPad, but, that's not actually possible. It's only because in that situation, the block-end must in fact have been unreachable.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79605