[PATCH] D64888: Use the MachineBasicBlock symbol for a callbr target

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:436
+              for (const MachineBasicBlock &MBB : *MF)
+                if (BB == MBB.getBasicBlock()) {
+                  MBB.getSymbol()->print(OS, AP->MAI);
----------------
void wrote:
> nickdesaulniers wrote:
> > I'm not sure that either side of this `if` are being tested/covered by your new test in callbr-asm-obj-file.ll.
> > 
> > In particular, the case that worries me is passing the address of a label as an input to `asm goto`.  Consider:
> > 
> > ```
> > asm goto(".quad %l0\n\t.quad %l1" :: "i"(&&baz) :: bar);
> >         bar:;
> >         baz:;
> > ```
> > 
> > https://godbolt.org/z/lKa_HD
> > 
> > where 2 blockaddresses are parameters to a callbr, one is the address of a label, one is the jump target list.
> > 
> > I'd be more comfortable signing off on code review if that case was also tested here (and considered in further changes related to `callbr`)
> We need to get clang to accept your code snippet first. :-)
> 
> I'm not sure I understand what you're saying here. What are you worried will be the outcome?
> 
> Despite the looping and whatnot, what this change does is fairly straightforward: instead of getting the IR basic block and creating a label for it, it uses the machine basic block's label directly. Similar to the code on line 436-437.
Sorry, maybe a better example:

```
int foo(void) {
    asm goto(".quad %l0\n\t.quad %l1" :: "i"(&&baz) :: bar);
    baz: return 1;
    bar: return -1;
}
```
before your patch `clang -O2 x.c -c -S` produces:
```
foo:                                    # @foo
# %bb.0:
	movl	$1, %eax
	#APP
	.quad .Ltmp0
	.quad .Ltmp1
	#NO_APP
	jmp	.LBB0_2
.Ltmp1:                                 # Block address taken
.LBB0_1:
	movl	$-1, %eax
.Ltmp0:                                 # Block address taken
.LBB0_2:
	retq
```
after:
```
foo:                                    # @foo
# %bb.0:
	movl	$1, %eax
	#APP
	.quad .LBB0_2
	.quad .LBB0_1
	#NO_APP
	jmp	.LBB0_2
.Ltmp0:                                 # Block address taken
.LBB0_1:
	movl	$-1, %eax
.Ltmp1:                                 # Block address taken
.LBB0_2:
	retq
```
So that looks like an improvement (and there's more we can do in a follow up to be better).

Before your patch `clang -O2 x.c -c` fails outright, with it, it succeeds and the disassembly looks good.  Ok, sorry for the noise, LGTM.

I think it would be good for us further to not emit the `.LtmpX` labels, since they both point to the same location.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64888