This is an archive of the discontinued LLVM Phabricator instance.

[Propeller]: Use a descriptive temporary symbol name for the end of the basic block.
ClosedPublic

Authored by rahmanl on Jul 15 2020, 9:20 AM.

Details

Summary

This patch changes the functionality of AsmPrinter to name the basic block end labels as LBB_END${i}_${j}, with ${i} being the identifier for the function and ${j} being the identifier for the basic block. The new naming scheme is consistent with how basic block labels are named (.LBB${i}_{j}), and how function end symbol are named (.Lfunc_end${i}) and helps to write stronger tests for the upcoming patch for BB-Info section (as proposed in https://lists.llvm.org/pipermail/llvm-dev/2020-July/143512.html). The end label is used with basicblock-labels (BB-Info section in future) and basicblock-sections to compute the size of basic blocks and basic block sections, respectively. For BB sections, the section containing the entry basic block will not have a BB end label since it already gets the function end-label.
This label is cached for every basic block (CachedEndMCSymbol) like the label for the basic block (CachedMCSymbol).

Diff Detail

Event Timeline

rahmanl created this revision.Jul 15 2020, 9:20 AM

Some inital comments. Expanding the summary to provide a bit more detail on why this symbol naming scheme is better might be useful. Thanks for doing this.

llvm/include/llvm/CodeGen/MachineBasicBlock.h
177

The comment "Used during basic block sections..." is still relevant?

478

Can we name it getCachedEndMCSymbol?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
3055

Remove this, it does not belong to this patch.

rahmanl edited the summary of this revision. (Show Details)Jul 17 2020, 1:32 AM
rahmanl added a reviewer: eli.friedman.
rahmanl updated this revision to Diff 278678.Jul 17 2020, 1:50 AM

Remove unrelated changes.

rahmanl updated this revision to Diff 278681.Jul 17 2020, 2:10 AM
rahmanl marked an inline comment as done.
rahmanl marked an inline comment as done.Jul 17 2020, 2:12 AM
rahmanl added inline comments.
llvm/include/llvm/CodeGen/MachineBasicBlock.h
478

This naming follows the similar function MachineBasicBlock::getSymbol() which also uses a cached symbol (called CachedMCSymbol).

shenhan added inline comments.Jul 20 2020, 10:07 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
479

If all usage of getEndSymbol() accept a const version of "MCSymbol *", maybe change it to "const MCSymbol *" that goes along with the const nature of the function.

rahmanl edited the summary of this revision. (Show Details)Jul 21 2020, 2:34 PM
rahmanl marked an inline comment as done.
rahmanl added inline comments.
llvm/include/llvm/CodeGen/MachineBasicBlock.h
479

Unfortunately, that's not the case. MCObjectStreamer::emitLabel takes a non-const reference to a symbol (and actually mutates the symbol).

rahmanl edited the summary of this revision. (Show Details)Jul 22 2020, 10:11 AM
rahmanl added a reviewer: snehasish.
rahmanl marked 3 inline comments as done.Aug 4 2020, 10:42 PM
rahmanl added a subscriber: MaskRay.

@eli.friedman and @MaskRay Does this small patch sound alright?

.Ltmp -> .LBB_END is a good idea. The assembly is more readable. There is no space hit because .L symbols are usually not emitted into the symbol table.

llvm/include/llvm/CodeGen/MachineBasicBlock.h
479

If you delete`const` from getEndSymbol() , can you delete the mutable specifier on CachedEndMCSymbol?

rahmanl added inline comments.Aug 4 2020, 11:43 PM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
479

That's possible. The current pattern is borrowed from MachineBasicBlock::getSymbol() and how it interacts with MachineBasicBlock::CachedMCSymbol*, which are respectively declared const and mutable. WDYT?

This patch is simple enough and LGTM.

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