[PATCH] D105916: [AsmPrinter][CallGraphSection] Emit call graph section

[Necip Fazil Yildiran via Phabricator via llvm-commits]

necipfazil added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:204
+
+  DenseMap<Function *, FunctionInfo> FunctionLabels;
+
----------------
MaskRay wrote:
> unstable iteration order. may consider MapVector https://llvm.org/docs/ProgrammersManual.html#llvm-adt-mapvector-h
We iterate over `FunctionLabels` only once when emitting the labels, and do not mutate the map while so.

If there is no other specific reason, we may keep using `DenseMap` as there is no need for emitting the functions in a certain order.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1388
+  SmallVector<MDNode *, 2> Types;
+  F.getMetadata(LLVMContext::MD_type, Types);
+  MDString *MDGeneralizedTypeId = nullptr;
----------------
morehouse wrote:
> Interesting.  So metadata persists on functions all the way to asm printing?  Do any LLVM optimizations drop this metadata?
Based on the experiments with SPEC CPU2006 483.xalancbmk, no metadata was dropped for -O{0,1,2,3}. However, I have no additional evidence to support whether the metadata is guaranteed to be preserved for functions.

Besides this, the current implementation does not set metadata for some functions in the first place. These include compiler intrinsics like `__clang_call_terminate`,  `asan.module_ctor`, `_GLOBAL__sub_I_*`. I have no other solution than handling these per-case at where LLVM functions are created for these. I will look into them shortly. On the other hand, these functions are not interesting for our use case.

To cover for all, the layout supports listing indirect targets with unknown type ids. We use it when we capture the indirect-targetness at the back-end, yet cannot find any type id metadata.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:1013
+  static unsigned UniqueID = 1;
+  return Ctx->getELFSection(".callgraph", ELF::SHT_PROGBITS,
+                            ELF::SHF_LINK_ORDER, 0, nullptr, false, UniqueID++,
----------------
MaskRay wrote:
> Use the style of getStackSizesSection. Pass in the MCSection &TextSec, and do SHF_LINK_ORDER/SHF_GROUP here, instead of at the call site.
I am not sure if I understand this. We need the function symbol for creating the section, to which we will link the section.  I am already doing SHF_LINK_ORDER at getCallGraphSection. Could you please elaborate?

Update:
@MaskRay, thanks for elaborating this per my help request in a chat. Per your suggestion, getCallGraphSection now sets SHF_GROUP in the text section by passing the text section as parameter.


================
Comment at: llvm/test/CodeGen/call-graph-section.ll:5
+; RUN: llvm-readelf -x .callgraph - | FileCheck %s
+; XFAIL: *
+
----------------
morehouse wrote:
> MaskRay wrote:
> > Why is this XFAIL?
> Yes, we should figure this out before landing.  Probably we need to fix an assumption in the `llc` parser that no longer holds due to the new operand bundle type.
I have fix for `llc` that I will shortly push with another revision.

Besides, `XFAIL` is now removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105916