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

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

There's some weird handling of fragment images, but I guess the idea is to implement them in future diffs - maybe https://reviews.llvm.org/D130521 ?  We need to coordinate this well so we don't lose track of everything that needs to be updated after this diff lands. I have some comments in the parts of the code that are reading a bit odd to me at the moment.



================
Comment at: bolt/lib/Core/BinaryEmitter.cpp:387
+  MCSymbol *EndSymbol = FF.isSplitFragment()
+                            ? Function.getFunctionColdEndLabel()
+                            : Function.getFunctionEndLabel();
----------------
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. 


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3147
         BC->deregisterSection(*ColdSection);
       Function->ColdCodeSectionName = std::string(getBOLTTextSectionName());
     }
----------------
Move outside of the loop / under "if Function->isSplit"?


================
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();
----------------
Emitting the same coldPart over and over for each fragment?


================
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);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130052