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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks, I have followed the flow. The logic looks good. But I have got some nits and a question about the `clang/test/` assembly test.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1033
+///  * 2nd bit: set if it is a block ending with a tail call.
+///  * 3rd bit: set if it is an exception handling (EH) pad.
+/// The remaining bits are zero.
----------------
1st 2nd 3rd may be confusing. You can just write: 1<<0, 1<<1, and 1<<2. Actually, you may just say: use 3 bits to encode information for more precise profile, (1) (2) (3).

The first two sentences can thus be simplified.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1047
+      getObjFileLowering().getBBAddrMapSection(*MF.getSection());
+  if (!BBAddrMapSection)
+    return;
----------------
BBAddrMapSection is always non-null. Delete the if.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1058
+  // Emit BB Information for each basic block in the funciton.
+  for (const auto &MBB : MF) {
+    const MCSymbol *MBBSymbol =
----------------
auto -> MachineBasicBlock


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels.ll:2
 ; Check the basic block sections labels option
-; RUN: llc < %s -mtriple=x86_64-pc-linux -function-sections -basic-block-sections=labels | FileCheck %s -check-prefix=LINUX-LABELS
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -function-sections -basic-block-sections=labels | FileCheck %s -check-prefix=LINUX-LABELS
 
----------------
`LINUX-LABELS` is probably not a good CHECK prefix. The feature is generic and not specific to linux (i.e. you could use `-mtriple=x86_64`)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408