[PATCH] D130127: [llvm][TailDuplicator] don't taildup isInlineAsmBrIndirectTargets

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a subscriber: MatzeB.
nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/TailDuplicator.cpp:804-805
+  // indirect target, we need to see if the edge from PredBB to TailBB is from
+  // an INLINEASM_BR in PredBB, and then also if that edge was from the
+  // indirect target list, fallthrough/default target, or potentially both. If
+  // it's both, TailDuplicator::tailDuplicate will remove the edge, corrupting
----------------
mingmingl wrote:
> If I'm reading correctly, the 2nd condition is "the edge is from the indirect target list && TailBB is a fall-through block of PredBB". Correct me if I'm reading this wrong.
> 
> How is a fall-through TailBB different from a non-fall-through TailBB when it comes to corrupted successor/predecessor list? (This is purely for my education and thanks for any enlightening point!)
> If I'm reading correctly, the 2nd condition is "the edge is from the indirect target list && TailBB is a fall-through block of PredBB". Correct me if I'm reading this wrong.

That's correct. If TailBB is a fallthrough block of PredBB, there is an edge from PredBB to TailBB.  If PredBB contains an INLINEASM_BR, and TailBB is in the indirect target list, there is also an edge from PredBB to TailBB.  TailDuplicator::tailDuplicate seems to make the assumption that MachineBasicBlocks can only have one edge from a predecessor to a successor, but that's not necessarily true, as my added test case demonstrates.  But maybe there is some invariant that MachineBasicBlocks cannot have more than one edge between the same blocks? Maybe @MatzeB would know if such an invariant exists?  While INLINEASM_BR is not a terminating instruction (today, it was, and might become so again), I can imagine a `jcc .Ltmp1; jmp .Ltmp1;` also getting messed up by  TailDuplicator::tailDuplicate here (since there would also be 2 edges from pred to succ in that case as well).

> How is a fall-through TailBB different from a non-fall-through TailBB when it comes to corrupted successor/predecessor list?

INLINEASM_BR has an explicit fallthough MBB. This may differ from what follows immediately after that MCinstr; there may be an explicit branch to another MBB, or no instruction (which I'd also call fallthrough, though that's implicit fallthrough as compared to INLINEASM_BR's explicit fallthrough).

This patch addresses a case of successor/predecessor list corruption stemming from TailDuplicator::tailDuplicate when TailBB is BOTH the explicit fallthrough of an INLINEASM_BR AND indirect target simultaneously.

> This is purely for my education and thanks for any enlightening point!

Always! Never hesitate to ask; if there's a way I could modify the wording of the comment or commit message to make anything clearer; I'm open to suggestions. Otherwise I'll land this when I get back from lunch in ~1hr. Thanks for taking the time to look over my patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130127