Use the MachineBasicBlock symbol for a callbr target

[void]

Summary

Inline asm doesn't use labels when compiled as an object file. Therefore, we
shouldn't create one for the (potential) callbr destination. Instead, use the
symbol for the MachineBasicBlock.

[xbolva00]

Test?

[void]

In D64888#1590331, @xbolva00 wrote:

Test?

On its way. :-)

[xbolva00]

Thanks

Fixes https://bugs.llvm.org/show_bug.cgi?id=42147 ?

[void]

Add testcase.

[void]

In D64888#1590354, @xbolva00 wrote:

Thanks

Fixes https://bugs.llvm.org/show_bug.cgi?id=42147 ?

Yes, I believe so.

[void]

Fix bad update.

[xbolva00]

test/CodeGen/X86/callbr-asm-obj-file.ll

4

; RUN:

[nickdesaulniers]

In D64888#1590354, @xbolva00 wrote:

Thanks

Fixes https://bugs.llvm.org/show_bug.cgi?id=42147 ?

I'm curious too, what exactly this solves, maybe a test will elucidate.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

435

range based for?

for (const MachineBasicBlock& MBB : MF)
...

might even be able to replace MF with the expression above and still fit on 80 lines.

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

433–439

getBasicBlock() would be a more explicit method to call, preferable to getOperand(1).

[void]

Fix syntax errors and use better accessor methods.

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

433–439

Good call! :-)

435

I tried that, but never was able to get the compiler to believe that MF had a "begin()" and "end()". Eventually just gave up... I'll try again though.

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

435

maybe *MF?

[void]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

435

That gives me this error *sigh*:

error: use of deleted function ‘llvm::MachineBasicBlock::MachineBasicBlock(const llvm::MachineBasicBlock&)’

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

435

That sounds like you forgot to make the iteration variable a reference, so a deleted copy constructor is being invoked.

diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
index 7721e996aca5..692877bdeb84 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
@@ -429,6 +429,10 @@ static void EmitGCCInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
           // PrintAsmOperand?
           if (Modifier[0] == 'l') { // Labels are target independent.
             if (MI->getOperand(OpNo).isBlockAddress()) {
+              const MachineFunction *MF = MI->getParent()->getParent();
+              for (const MachineBasicBlock& MBB : *MF) {
+                errs() << "XXX\n";
+              }

compiles just fine for me.

[void]

Use for-each loop.

[nickdesaulniers]

test/CodeGen/X86/callbr-asm-obj-file.ll

8

Can you add

; CHECK-NOT:

cases so I get a better idea of what LLVM used to print before this change?
(So we have an affirmative test case, and one negative test case)

[void]

test/CodeGen/X86/callbr-asm-obj-file.ll

8

It used to emit an error with no output.

[nickdesaulniers]

test/CodeGen/X86/callbr-asm-obj-file.ll

2

is -pc-linux actually recognized? I would have expected x86_64-linux-gnu?

[void]

Update triple.

[void]

Update test with correct dest labels

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

436

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)

[void]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

436

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.

[void]

Git-ifying my change.

[nickdesaulniers]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

436

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.

[void]

Thanks! Though your example pointed out a separate problem: not accepting the label as a valid operand. I'll do some work on that.

[xbolva00]

It would be nice to merge this fix into 9.0 branch after some time in trunk. What do you think?

@hans @void

[nickdesaulniers]

In D64888#1593116, @xbolva00 wrote:

It would be nice to merge this fix into 9.0 branch after some time in trunk. What do you think?

@hans @void

So two regressions were immediately reported:

1. https://github.com/ClangBuiltLinux/linux/issues/614
2. https://github.com/ClangBuiltLinux/linux/issues/615

so honestly I'd rather revert it until we understand the exact issue better.

@void has also authored https://reviews.llvm.org/D64982.

For the life of me I swear there's a bug that @craig.topper commented on that explains an issue with symbols not being added to the symbol table at the MC layer, but I cannot find it (and it's killing me). I think that should be pursued (why were we generating a .tmp symbol anyways).

[nickdesaulniers]

In D64888#1593167, @nickdesaulniers wrote:

For the life of me I swear there's a bug that @craig.topper commented on that explains an issue with symbols not being added to the symbol table at the MC layer, but I cannot find it (and it's killing me). I think that should be pursued (why were we generating a .tmp symbol anyways).

https://bugs.llvm.org/show_bug.cgi?id=42147

[nickdesaulniers]

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

434 ↗

(On Diff #210719)

So another issue I though of with this approach:

It makes sense to me as implemented; we need some mapping from BlockAddress of the BasicBlock to the corresponding MachineBasicBlock (we have the reverse mapping; given a MachineBasicBlock we can get the BasicBlock it comes from, but we don't have the reverse mapping; GetBlockAddressSymbol kind of does this but has other issues). This approach works for the test case provided, and probably all C/C++ that could ever hit this path BUT it is not safe to assume that the MachineBasicBlock corresponding to the BlockAddress is within this MachineFunction.

Stated another way, I could have blockaddress(@foo, %bar) used in MachineFunction @baz. The code in this patch would iterate the MachineBasicBlocks of the MachineFunction corresponding to @baz since the MachineInstruction using the blockaddress constant was in @baz. It would never find %bar since it needs to iterate MachineBasicBlocks of @foo, not @baz.

I think this would be fixed by initializing MF to BB->getParent().

[nickdesaulniers]

Reverted in https://reviews.llvm.org/rL366600. While GetBlockAddressSymbol has some complicated machinery related to managing symbols of temporary addresses, I get the feeling a complete fix will lie in modifications there.

[hans]

In D64888#1593116, @xbolva00 wrote:

It would be nice to merge this fix into 9.0 branch after some time in trunk. What do you think?

@hans

Sounds like this was reverted on trunk. Please let me know if there are follow-ups that might be candidates for merging.

[void]

In D64888#1596336, @hans wrote:

In D64888#1593116, @xbolva00 wrote:

It would be nice to merge this fix into 9.0 branch after some time in trunk. What do you think?

@hans

Sounds like this was reverted on trunk. Please let me know if there are follow-ups that might be candidates for merging.

I have a patch in my local branch that should fix some of the issues. However, I can't seem to support the situation Nick pointed out. If you have something like:

define void @foo() {
entry:
  callbr void asm sideeffect ".word ${0:l}", "X"(i8* blockaddress(@bar, %next))
          to label %if.end10 [label %l_yes]

if.end10:
  br label %l_yes

l_yes:
  ret void
}

define i32 @bar() {
entry:
  br label %next

next:
  ret i32 42
}

This will fail, because @bar hasn't been created yet, so it can't get the address of next. Or, rather, it *can*, but the assembly parser won't realize that it's a label and will create a new label for it. So it goes from this:

foo:
    .word .Ltmp0
  ...
bar:
.Ltmp0:

to this:

foo:
    .word .Ltmp00  ; <- Notice the '0' suffix, which is applied to a label to uniquify it.
  ...
bar:
.Ltmp0:

when parsing the inline assembly. I *think* we need to convince MCContext that .Ltmp0 is a label that it doesn't need to uniquify—i.e., register it as a symbol in MCContext instead of treating it as a temporary label (there's a difference, and I'm not sure why).

The question is whether this is a case we need to care about at this point in time.

Thoughts?

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

case '$': {
           unsigned OpFlags = MI->getOperand(OpNo).getImm();
           ++OpNo;  // Skip over the ID number.
 
           // FIXME: Shouldn't arch-independent output template handling go into
           // PrintAsmOperand?
           if (Modifier[0] == 'l') { // Labels are target independent.
             if (MI->getOperand(OpNo).isBlockAddress()) {
               const BlockAddress *BA = MI->getOperand(OpNo).getBlockAddress();
-              MCSymbol *Sym = AP->GetBlockAddressSymbol(BA);
-              Sym->print(OS, AP->MAI);
+              const BasicBlock *BB = BA->getBasicBlock();
+              const MachineFunction *MF = MI->getParent()->getParent();
+              for (const MachineBasicBlock &MBB : *MF)
+                if (BB == MBB.getBasicBlock()) {
+                  MBB.getSymbol()->print(OS, AP->MAI);
+                  break;
+                }
             } else if (MI->getOperand(OpNo).isMBB()) {
               const MCSymbol *Sym = MI->getOperand(OpNo).getMBB()->getSymbol();
               Sym->print(OS, AP->MAI);
             } else {
               Error = true;
             }
           } else {
             if (InlineAsm::isMemKind(OpFlags)) {

test/CodeGen/X86/callbr-asm-obj-file.ll

+; RUN: llc < %s -mtriple=x86_64-linux-gnu -filetype=obj -o - \
+; RUN:  | llvm-objdump -triple x86_64-linux-gnu -d - \
+; RUN:  | FileCheck %s
+
+; CHECK: 0000000000000000 test1:
+; CHECK-NEXT:   0: 74 00 je 0 <test1+0x2>
+; CHECK-NEXT:   2: c3    retq
+
+define void @test1() {
+entry:
+  callbr void asm sideeffect "je ${0:l}", "X,~{dirflag},~{fpsr},~{flags}"(i8* blockaddress(@test1, %a.b.normal.jump))
+          to label %asm.fallthrough [label %a.b.normal.jump]
+
+asm.fallthrough:
+  ret void
+
+a.b.normal.jump:
+  ret void
+}

test/CodeGen/X86/callbr-asm.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-- -O3 | FileCheck %s
 
 ; Tests for using callbr as an asm-goto wrapper
 
 ; Test 1 - fallthrough label gets removed, but the fallthrough code that is
 ; unreachable due to asm ending on a jmp is still left in.
 define i32 @test1(i32 %a) {
 ; CHECK-LABEL: test1:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; CHECK-NEXT:    addl $4, %eax
 ; CHECK-NEXT:    #APP
 ; CHECK-NEXT:    xorl %eax, %eax
-; CHECK-NEXT:    jmp .Ltmp00
+; CHECK-NEXT:    jmp .LBB0_2
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB0_1: # %normal
 ; CHECK-NEXT:    xorl %eax, %eax
 ; CHECK-NEXT:    retl
 ; CHECK-NEXT:  .Ltmp0: # Block address taken
 ; CHECK-NEXT:  .LBB0_2: # %fail
 ; CHECK-NEXT:    movl $1, %eax
 ; CHECK-NEXT:    retl

 ; CHECK-NEXT:    .p2align 4, 0x90
 ; CHECK-NEXT:  .Ltmp4: # Block address taken
 ; CHECK-NEXT:  .LBB2_4: # %label04
 ; CHECK-NEXT:    # Parent Loop BB2_1 Depth=1
 ; CHECK-NEXT:    # Parent Loop BB2_2 Depth=2
 ; CHECK-NEXT:    # Parent Loop BB2_3 Depth=3
 ; CHECK-NEXT:    # => This Inner Loop Header: Depth=4
 ; CHECK-NEXT:    #APP
-; CHECK-NEXT:    jmp .Ltmp10
-; CHECK-NEXT:    jmp .Ltmp20
-; CHECK-NEXT:    jmp .Ltmp30
+; CHECK-NEXT:    jmp .LBB2_1
+; CHECK-NEXT:    jmp .LBB2_2
+; CHECK-NEXT:    jmp .LBB2_3
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB2_5: # %normal0
 ; CHECK-NEXT:    # in Loop: Header=BB2_4 Depth=4
 ; CHECK-NEXT:    #APP
-; CHECK-NEXT:    jmp .Ltmp10
-; CHECK-NEXT:    jmp .Ltmp20
-; CHECK-NEXT:    jmp .Ltmp30
-; CHECK-NEXT:    jmp .Ltmp40
+; CHECK-NEXT:    jmp .LBB2_1
+; CHECK-NEXT:    jmp .LBB2_2
+; CHECK-NEXT:    jmp .LBB2_3
+; CHECK-NEXT:    jmp .LBB2_4
 ; CHECK-NEXT:    #NO_APP
 ; CHECK-NEXT:  .LBB2_6: # %normal1
 ; CHECK-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; CHECK-NEXT:    retl
 entry:
   %a.addr = alloca i32, align 4
   store i32 %a, i32* %a.addr, align 4
   br label %label01