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

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
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:
> ayermolo wrote:
> > 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.
> I was thinking the factored out code would be something like:
> 
> ```
> std::vector<std::pair<Elf_Shdr*, Elf_Shdr*>> getSectionAndRelocations(std::function<bool(Elf_Shdr*)> IsMatch) {/*...*/}
> ```
> 
> where each of the different users would provide their own predicate (i.e. name or type check in the cg profile and stack sizes cases). Types in the example are just a rough idea. In the cgprofile case, we can just pick one pair from the vector.
Ah yeah, after I re-wrote printCGProfile yesterday evening, I see your point. The loop ended up being basically same as in printRelocatableStackSizes, except for checks. Will refactor it in to helper function. Also made it so that relocation section is optional. Will print warning if it's missing, but continue output weights. So some of the yaml tests can be cleaned up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104080