[PATCH] D81775: [COFF] Add cg_profile directive and .llvm.call-graph-profile section

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/MC/MCParser/COFFAsmParser.cpp:336
+
+  // Only store non-temporary symbols.
+  if (!FromSym->isTemporary() && !ToSym->isTemporary())
----------------
zequanwu wrote:
> MaskRay wrote:
> > zequanwu wrote:
> > > Only store non-temporary symbols pairs.
> > > 
> > > In ELF, temporary symbols are also stored. I don't get the point of doing that. As I know, temporary symbols will not show up in object file. So, there is no reason to store temporary symbols.
> > Temporary symbols are a subcategory of local symbols. I think in ELF, it does not really matter whether a profile edge references a local symbol or not, a temporary symbol or not. A temporary symbol can also encode a profile edge.
> > 
> > Is there any particular reason you want to exclude temporary symbols from COFF?
> From my understanding, since temporary symbols doesn't show up in final binary and the purpose of `.llvm.call-graph-profile`  is to be used to optimize section layouts of final binary, storing temporary symbols doesn't help. 
Please drop isTemporary.

For temporary symbols (`.L` prefixed), the streamer can use a symbol representing the defined section. In ELF, this can cause creation of STT_SECTION. See D51047 for a COFF example.


================
Comment at: llvm/lib/MC/MCWinCOFFStreamer.cpp:342
+  if (Created) {
+    cast<MCSymbolCOFF>(S)->setIsWeakExternal();
+    cast<MCSymbolCOFF>(S)->setExternal(true);
----------------
zequanwu wrote:
> MaskRay wrote:
> > Why setIsWeakExternal?
> Doing the same as `finalizeCGProfileEntry` in ELF, https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCELFStreamer.cpp#L483.
OK.

http://llvm.org/docs/Extensions.html#sht-llvm-call-graph-profile-section-call-graph-profile says that the undefined symbol is weak. It does not have to be specified that way...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81775