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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
Comment at: llvm/lib/MC/MCELFStreamer.cpp:498
+  MCObjectStreamer::emitRelocDirective(
+      *MCOffset, getAssembler().getBackend().getNoneRelcationName(), SRE,
+      SRE->getLoc(), *getContext().getSubtargetInfo());
----------------
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)


================
Comment at: llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.h:53
+    llvm_unreachable("getNoneRelcationName() unimplemented");
+    return "";
+  }
----------------
MaskRay wrote:
> unreachable doesn't need `return`
I was following example of fixupNeedsRelaxation. Will remove.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:1857-1864
+  // Following LLD lead and doing a seperate loop in case relocation section
+  // comes before section it applies to.
+  for (const Elf_Shdr &Sec : Sections) {
+    if (Sec.sh_info == DotCGProfileSecIndex) {
+      DotCGProfileSecRela = &Sec;
+      break;
+    }
----------------
jhenderson wrote:
> This code will only allow one .cg_profile section per object, which may or may not be appropriate (it's true that this was the case before, but was it correct then?). Either way, we already have `printRelocatableStackSizes` which finds all `.stack_sizes` sections and their corresponding relocation sections. Maybe rather than having two similar pieces of code that do basically the same thing, but in different ways, we should combine them?
Hmm, well from my understanding if it comes from clang side it should be one per object file. I guess someone can construct object with multiple ones, using say yaml2obj, but what would be practical point of it? LLD right now doesn't support that scenario.
I looked in to printRelocatableStackSizes, not sure there is enough code worth combining, but seems like a good way to keep all the code related to printing cg profile in one place. Let me refactor and see what comes out of it.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6738-6739
+  }
+  assert(CGProfileRelaOrError->size() == (CGProfileOrErr->size() * 2) &&
+         "Number of from/to pairs does not match number of frequencies.");
+
----------------
jhenderson wrote:
> Seems like this shouldn't be an assert, but a proper warning? If you create a broken object with the wrong number of relocations, this assertion will fire.
I can change it to warning, but then will need to add a check not to execute a loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080