[PATCH] D130316: [SelectionDAG] make INLINEASM_BR use MachineBasicBlocks instead of BlockAddresses

[Bill Wendling via Phabricator via llvm-commits]

void accepted this revision.
void added a comment.
This revision is now accepted and ready to land.

This patch seems pretty straight-forward, so I'll add my LGTM here. Please feel free to override that if other reviewers are less sure. :-)



================
Comment at: llvm/test/CodeGen/AArch64/callbr-asm-label.ll:10
 ; CHECK: // %bb.1:
-; CHECK: .Ltmp0:
+; CHECK: .Ltmp1:
 ; CHECK: .LBB0_2: // %indirect
----------------
nickdesaulniers wrote:
> barannikov88 wrote:
> > I'm a bit out of context, so the question might be silly, sorry.
> > Shouldn't this temporary label no longer be emitted? It used to be referenced, but no longer is.
> > 
> It's not a silly question; it's a great question.
> 
> I've been trying to track that down, as I don't know the answer myself.
> 
> IIRC, we did previously have an issue with labels disappearing out from under us. @void solved that in either
> 
> - commit 83d690a14980 ("Don't rely on 'l'(ell) modifiers to indicate a label reference")
> - commit 1b104388752f ("[MC] Don't recreate a label if it's already used")
> 
> I've already found that the latter is no longer necessary, hence https://reviews.llvm.org/D130323, but that's not what was retaining these additional symbols.
> 
> (or maybe it was a third commit, maybe 79176a2542d03107b90613c84f18ccba41ad8fa8?)
> 
> So I would like to find the answer to this question, and perhaps remove the duplicate labels.  But it would be a minor cosmetic fix; I'm not going to spend too much time on it since this patch is just a chain in a longer series that we're ultimately trying to fix.  Fixing duplicate labels is nice to have, but slightly orthogonal.
My memory is hazy on the details, but there were a number of issues with inline asm and labels when dealing with block addresses and basic blocks. The changes you mentioned are large hammers, but *seemed* necessary. There may be better ways to handle the label issue though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130316