[PATCH] D130052: [BOLT] Support passing fragments to code emission

[Fabian Parzefall via Phabricator via llvm-commits]

FPar added a comment.

I am aware that the ordering of diffs not ideal. The diffs D130052 <https://reviews.llvm.org/D130052>, D130520 <https://reviews.llvm.org/D130520> and D130521 <https://reviews.llvm.org/D130521> are the baseline of changes required to get simple programs running. I tried to group related changes, but they all depend on each other in some way.



================
Comment at: bolt/lib/Core/BinaryEmitter.cpp:387
+  MCSymbol *EndSymbol = FF.isSplitFragment()
+                            ? Function.getFunctionColdEndLabel()
+                            : Function.getFunctionEndLabel();
----------------
rafauler wrote:
> rafauler wrote:
> > I guess the fact that we don't have individual end labels for each fragment is going to be a problem for determining the correct size of these symbols in emitELFSize. But according to 247b4181a306aff348c1d2d8c3847280866a7f32, this symbol table isn't terribly important. But I would rather not have incorrect code. 
> I see this is implemented in https://reviews.llvm.org/D130520 now.
Correct.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3147
         BC->deregisterSection(*ColdSection);
       Function->ColdCodeSectionName = std::string(getBOLTTextSectionName());
     }
----------------
rafauler wrote:
> Move outside of the loop / under "if Function->isSplit"?
Moved to `getLayout().isSplit()`, because `Function->isSplit()` is a slightly different check.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3684-3715
+    for (const FunctionFragment FF : Function.getLayout().getSplitFragments()) {
+      ErrorOr<BinarySection &> ColdSection =
+          Function.getColdCodeSection(FF.getFragmentNum());
+      assert(ColdSection && "cannot find section for cold part");
+      // Cold fragments are aligned at 16 bytes.
+      NextAvailableAddress = alignTo(NextAvailableAddress, 16);
+      BinaryFunction::FragmentInfo &ColdPart = Function.cold();
----------------
rafauler wrote:
> Emitting the same coldPart over and over for each fragment?
This is addressed in D132051. This diff only takes care that `ColdSection` has the right output address.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4485-4486
+            Function.getColdCodeSection(FF.getFragmentNum())->getIndex();
+        NewColdSym.st_value = Function.cold().getAddress();
+        NewColdSym.st_size = Function.cold().getImageSize();
+        NewColdSym.setBindingAndType(ELF::STB_LOCAL, ELF::STT_FUNC);
----------------
rafauler wrote:
> Same image for all fragments? I guess this will be implemented in a follow-up diff?  ps:  I don't see that implemented in https://reviews.llvm.org/D130521
Yes. Again, D132051.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130052