[PATCH] D85408: Let -basic-block-sections=labels emit basicblock metadata in a new .bb_addr_map section, instead of emitting special unary-encoded symbols.

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1030
+/// recursive return edges vs. indirect branches. The format of the metadata is
+/// described as follows: 1st bit (LSB): set if this is a return block (return
+/// or tail call). 2nd bit: set if this is a block ending with a tail call. 3rd
----------------
Did you intend to change the formatting of the comment? I think the list items were on separate lines.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:58
 
+unsigned MachineBasicBlock::getBBAddrMapMetadata() const {
+  const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();
----------------
rahmanl wrote:
> snehasish wrote:
> > I think this method only relies on already public methods of MachineBasicBlock. It would be cleaner to move this from here to a static helper function in AsmPrinter.cpp. This way we don't add yet another method to the MachineBasicBlock class and keeps the relevant metadata generation logic close to where it is used.
> > 
> > Feel free to ignore if you plan on calling this method elsewhere apart from AsmPrinter.cpp but I can't think of a reason to do so.
> Thanks for the suggestion. It makes the code much more readable since I just place that function above emitBBAddrMapSection.
> I can even make it a lambda function within emitBBAddrMapSection. WDYT?
I have a slight preference for the static method vs inline lambda since the comment describing the metadata can be placed above the static method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408