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

Wed Aug 31 14:27:03 PDT 2022

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:
> 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.
Oh, I'm mixing up the MCInstr (lower level IR) with the Instruction (higher level IR). INLINEASM_BR (MCInstr) does not have an explicit fallthrough. callbr (Instruction) does.

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

That wasn't the specific case I was referring to, but I think the answer for that case would be still be "yes."

i.e.
```
%bb.4:
INLINEASM_BR &"", 1 /* sideeffect attdialect */, 13 /* imm */, %bb.5
JMP_1 %bb.5
```
so there's 2 edges from Pred (%bb.4) to Succ (%bb.5).  It's probably ok for taildup to subsume the contents of %bb.5 into %bb.4, removing the `JMP`, but it should not remove %bb.5 from the successor list, because INLINEASM_BR still exists and hasn't been updated.  I'm not sure yet if that transform is even profitable or not.

> does fall-through really mean the edge (that connects basic blocks) here?

Yes, I believe MachineBasicBlocks don't need explicit terminators and may implicitly fallthrough. I might be mis-remembering though.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130127