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

[Mingming Liu via Phabricator via llvm-commits]

mingmingl 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
----------------
nickdesaulniers wrote:
> 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.
> 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.

Ah I was also assuming there is only one edge from Pred to each one of its Succ. This is helpful!

> 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.

To confirm my understanding, when TailBB is both the explicit fall through and implicit fall-through, tail-duplicate eliminate both edges (from PredBB to TailBB, and from INLINEASM_BR to TailBB) while INLINEASM_BR edge should be kept, is that correct? (if yes, I think I understand the issue better!)

I'm not a fan of terms, but just to clarify a little bit, does fall-through really mean the edge (that connects basic blocks) here? Ask since I originally thought fall-through indicates layout (e.g., TailBB is placed right after PredBB in the generated machine function) -> in that sense, whether TailBB is placed right after PredBB or not (i.e., whether TailBB is a layout fall-through or not), TailBB shouldn't be tail duplicated (otherwise, both edges are deleted and issue happen).

> 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. 

Your explanation is much appreciated! Thanks for taking the time to reply!

It took me sometime to wrap my head and figure out what i missed, so don't bother with wording it again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130127