This is an archive of the discontinued LLVM Phabricator instance.

[MC] Don't recreate a label if it's already used
ClosedPublic

Authored by void on Jul 25 2019, 3:48 PM.

Details

Summary

This patch keeps track of MCSymbols created for blocks that were
referenced in inline asm. It prevents creating a new symbol which
doesn't refer to the block.

Inline asm may have a reference to a label. The asm parser however
doesn't recognize it as a label and tries to create a new symbol. The
result being that instead of the original symbol (e.g. ".Ltmp0") the
parser replaces it in the inline asm with the new one (e.g. ".Ltmp00")
without updating it in the symbol table. So the machine basic block
retains the "old" symbol (".Ltmp0"), but the inline asm uses the new one
(".Ltmp00").

Diff Detail

Event Timeline

void created this revision.Jul 25 2019, 3:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 3:48 PM
void added a comment.Aug 7 2019, 11:23 AM

Friendly ping. :-)

void retitled this revision from Don't recreate a label if it's already used to [MC] Don't recreate a label if it's already used.Aug 7 2019, 10:12 PM
nickdesaulniers added inline comments.Aug 8 2019, 9:37 AM
lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp
435

Seeing repeated references to a the same variable in a parameter list (Sym) is a code smell. So registerInlineAsmLabel could just take a MCSymbol* and pull the name out of it via getName().

test/CodeGen/AArch64/callbr-asm-label.ll
1

the triple for AArch64 should be aarch64-linux-gnu (armv4t is 32b ARM).

test/CodeGen/X86/callbr-asm-label-addr.ll
8

Maybe not necessary to solve in this patch, but it's curious to me why we emit BOTH .Ltmp1 AND .LBB0_1 for %bar, but only .Ltmp0 for %baz (with %bb.2 commented out). Obviously related to %bar being the callbr jump target (while %baz is simply the address of a label), but something seems wrong to me to emit so many labels to the same statement.

From this case specifically, I feel like .LBB0_1: should not be emitted, and instead there should be a (commented out) # %bb.1: # %bar.

lib/MC/MCContext.cpp
64

I think we may want to be invoking the StringMap constructor that additionally takes an InitialSize, which we'd set to 0U, as it's highly unlikely that the %l modifier is used at all for any given piece of code.

IIUC, StringMap's constructor that simply accepts an allocator will allocate room for 1 instance. See the definition of StringMapImpl::StringMapImpl(unsigned InitSize, unsigned itemSize).

I think that micro optimization would save many small allocations for the general case.

void updated this revision to Diff 214217.Aug 8 2019, 1:14 PM
void marked 6 inline comments as done.

Remove smelly code and use correct triple in test.

void added inline comments.Aug 8 2019, 1:15 PM
lib/MC/MCContext.cpp
64

The c'tor that's called when we supply only an allocator is this one:

explicit StringMapImpl(unsigned itemSize)
      : ItemSize(itemSize) {}

If we supply an InitSize, it executes the init() function during creation, but only if it's non-zero. So the effect is the same whether we supply it or not.

test/CodeGen/X86/callbr-asm-label-addr.ll
8

Yeah, that's beyond the scope of this patch. It should *probably* emit a label only when needed, but that's probably infeasible with the current structure of MC...

void updated this revision to Diff 214299.Aug 8 2019, 10:24 PM

Update testcase.

void updated this revision to Diff 214430.Aug 9 2019, 1:11 PM

Move method out-of-line to keep MCContext.h simpler.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 9 2019, 1:15 PM
This revision was automatically updated to reflect the committed changes.