[PATCH] D104080: [LLD][LLVM] CG Graph profile using relocations

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Mostly looks good.



================
Comment at: lld/ELF/Driver.cpp:866
     auto *obj = cast<ObjFile<ELFT>>(file);
-
-    for (const Elf_CGProfile_Impl<ELFT> &cgpe : obj->cgProfile) {
-      auto *fromSym = dyn_cast<Defined>(&obj->getSymbol(cgpe.cgp_from));
-      auto *toSym = dyn_cast<Defined>(&obj->getSymbol(cgpe.cgp_to));
+    assert(obj->cgProfileRela.size() == obj->cgProfile.size() * 2 &&
+           "Number of relocations doesn't match Weights.");
----------------
This can happen so assert is not appropriate. You can use `if (...) fatal`


================
Comment at: llvm/include/llvm/MC/MCAsmBackend.h:88
+  /// Returns Backend specific name of _NONE relocation.
+  virtual StringRef getNoneRelcationName() const = 0;
+
----------------
Delete


================
Comment at: llvm/lib/MC/MCELFStreamer.cpp:498
+  MCObjectStreamer::emitRelocDirective(
+      *MCOffset, getAssembler().getBackend().getNoneRelcationName(), SRE,
+      SRE->getLoc(), *getContext().getSubtargetInfo());
----------------
ayermolo wrote:
> MaskRay wrote:
> > it's ugly, but you can use `.reloc xxx, BFD_RELOC_NONE, sym`
> This will reduce the scope of changes in this patch, but then new backend creator will need to know of this trick. I don't know the history of BFD_RELOC_NONE. Is it expected for architectures that have _NONE to have the mapping?
> . .Case("BFD_RELOC_NONE", ELF::R_<arch>_NONE)
> Is it expected for architectures that have _NONE to have the mapping? . .Case("BFD_RELOC_NONE", ELF::R_<arch>_NONE)

Yes. In GNU as, `BFD_RELOC_NONE` is generic and can be used on all targets. I have added `BFD_RELOC_NONE` to many LLVM targets. Note: .llvm.call-graph-profile is used by -fprofile-use=. The targets which don't support `BFD_RELOC_NONE` are not used by people for PGO. (If they do use PGO, adding the support should be trivial anyway.)

It probably makes sense to save the return value in a variable and assert it is `None`. I find that an unknown `BFD_RELOC_NONE` does not cause an error.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:677
 
+StringRef X86AsmBackend::getNoneRelcationName() const {
+  StringRef Name = "";
----------------
This function and its friends can all be deleted.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6752
+    ListScope L(W, "CGProfile");
+    for (uint32_t I = 0, Size = CGProfileOrErr->size(); I < Size; ++I) {
+      const Elf_CGProfile &CGPE = (*CGProfileOrErr)[I];
----------------
`I != Size` is probably more common in LLVM code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080