[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: clang/docs/UsersManual.rst:1706
+  its own unique section. With the "labels" value, normal sections are emitted,
+  but a ".bb_addr_map" section is emitted which would include address offsets
+  for each basic block in the program, relative to the function address.
----------------
nit: s/would include/includes/


================
Comment at: clang/docs/UsersManual.rst:1707
+  but a ".bb_addr_map" section is emitted which would include address offsets
+  for each basic block in the program, relative to the function address.
 
----------------
nit: s/relative to the/relative to the parent/


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:58
 
+unsigned MachineBasicBlock::getBBAddrMapMetadata() const {
+  const TargetInstrInfo *TII = getParent()->getSubtarget().getInstrInfo();
----------------
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.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-labels.ll:5-9
   %2 = alloca i8, align 1
   %3 = zext i1 %0 to i8
   store i8 %3, i8* %2, align 1
   %4 = load i8, i8* %2, align 1
   %5 = trunc i8 %4 to i1
----------------
I think this can be removed and the branch on L10 can directly use the param like so:

```
define void @_Z3bazb(i1 zeroext %0) personality i32 (...)* @__gxx_personality_v0 {
  br i1 %0, label %6, label %11
  ....
}
```

You will need to adjust the respective virtual register names since %2-%5 is no longer used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85408