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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added a comment.

The AsmPrinter stuff seems overly complicated by a non-optimal data structure choice.

Rather than two maps (`FuncEntryLabels`, `CallSiteLabels`) that get rearranged later, how about a single structure that captures the grouping we want? 
Something like:

  struct FunctionInfo {
    MCSymbol *EntryLabel;  // nullptr if this function is not a potential indirect target
    DenseMap<CGTypeId, MCSymbol *> CallSiteLabels;
  };
  DenseMap<Function *, FunctionInfo> FunctionLabels;

Then we can simply iterate this structure once to emit the call graph sections.  Inside the loop we can obtain the section for the current `Function *` key, compute its type ID if `EntryLabel != nullptr`, and emit the appropriate labels.
I think this would cut out a lot of code.



================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:813
+      const DenseMap<CGTypeIdVal, std::vector<const MCSymbol *>>
+          &CallSiteLabels);
 
----------------
I'm a little confused since `FuncEntryLabels` and `CallSiteLabels` are class members that I wouldn't expect need to be passed as parameters.  But  the types are different here.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1324
+    const auto &FuncTypeId = std::get<0>(El.second);
+    const auto &FuncEntryLabel = std::get<1>(El.second);
+    FuncEntryLabelsPerSection[CGSection][FuncTypeId].push_back(FuncEntryLabel);
----------------
Nit:  `El.second.first` and `El.second.second` seem clearer to me.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1336
+      const auto &TypeId = El2.first;
+      const auto &Labels = El2.second;
+      std::copy(
----------------
This is confusing since we now have both a type and a variable named `Labels`.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1341
+    }
+  }
+
----------------
This whole loop seems a little pointless.  We're copying from a `Func -> TypeId -> Labels` map to a `FuncSection -> TypeId -> Labels` map.  Where `FuncSection` is directly attainable if we have `Func`.

Actually, the other loop also seems unnecessary.  It is restructuring from `Func -> {TypeId, Label}` map to a `FuncSection -> TypeId -> Label` map.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1388
+  SmallVector<MDNode *, 2> Types;
+  F.getMetadata(LLVMContext::MD_type, Types);
+  MDString *MDGeneralizedTypeId = nullptr;
----------------
Interesting.  So metadata persists on functions all the way to asm printing?  Do any LLVM optimizations drop this metadata?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1406
+  return cast<ConstantInt>(ConstantInt::get(Int64Ty, TypeIdVal));
+}
+
----------------
Can we share this logic with MachineFunction.h?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1446
+        Func.hasAddressTaken(nullptr,
+                             /* IgnoreCallbackUses */ true,
+                             /* IgnoreAssumeLikeCalls */ true,
----------------
Why is it safe to ignore callbacks?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1455-1456
+        auto TypeIdVal = TypeId->getZExtValue();
+        FuncEntryLabels[&MF->getFunction()] =
+            std::pair<uint64_t, MCSymbol *>(TypeIdVal, S);
+      } else {
----------------



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1463-1464
+        // typeid (0).
+        FuncEntryLabels[&MF->getFunction()] =
+            std::pair<uint64_t, MCSymbol *>(0, S);
+      }
----------------



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1559
+      // resulting in emitting labels for them. The extra information can
+      // be neglected while disassembly but still takes space in the binary.
+      if (TM.Options.EmitCallGraphSection && MI.isCall()) {
----------------



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1564
+        if (CallSiteInfo != CallSitesInfoMap.end()) {
+          if (auto *TypeId = CallSitesInfoMap.find(&MI)->second.TypeId) {
+            // Emit label for the indirect call.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105916