[PATCH] D87813: [llvm]Add an option to emit cold clusters to a different section.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:82
 
+cl::opt<std::string> ColdSectionTextPrefix(
+    "bbsections-cold-prefix",
----------------
Worth a comment why the toggle exists.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:84
+    "bbsections-cold-prefix",
+    cl::desc("The text prefix to use for cold basic block clusters."),
+    cl::init(".text.unlikely."), cl::Hidden);
----------------
The description usually does not end with a full stop


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:69
 
+extern cl::opt<std::string> ColdSectionTextPrefix;
+
----------------
This is probably better declared in a header and named something with `BB` as a prefix, otherwise it can lead to confusion that this applies to the generic `.text.unlikely`


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-cold.ll:39
+; LINUX-SPLIT: .section	.text.split._Z3bazb,"ax", at progbits
+; LINUX-SPLIT: _Z3bazb.cold:
----------------
If `-NEXT:` applies please add it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87813