[PATCH] D132051: [BOLT] Track fragment info for all split fragments

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks Fabian! Overall, very nice work, although I feel that the diff stack in general has been a bit light on tests that guarantee that the changes have the intended effect. I suggest one test in one of the comments below.



================
Comment at: bolt/lib/Core/FunctionLayout.cpp:207-212
+  // Keep the main fragment around, because it may contain emission information
+  // even if the function is ignored.
+  std::for_each(Fragments.begin() + 1, Fragments.end(),
+                [](FunctionFragment *const FF) { delete FF; });
+  Fragments = FragmentListType{Fragments.front()};
+  getMainFragment().Size = 0;
----------------
Can you explain why was that added a bit better?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3685-3714
+    for (FunctionFragment &FF : Function.getLayout().getSplitFragments()) {
       ErrorOr<BinarySection &> ColdSection =
           Function.getCodeSection(FF.getFragmentNum());
       assert(ColdSection && "cannot find section for cold part");
       // Cold fragments are aligned at 16 bytes.
       NextAvailableAddress = alignTo(NextAvailableAddress, 16);
       if (TooLarge) {
----------------
This will write fragments in this order, assuming we have two funcs with three fragments each:

Func1.main_frag Func2.main_frag Func1.frag2 Func1.frag3 Func2.frag2 Func2.frag3

I'm not sure this is what you want? Aren't you looking for grouping all fragments N together?  (bear in mind that this code only runs for non-reloc mode, I think reloc mode is correct in grouping all fragments together)


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4477-4491
+        if (FF.getAddress()) {
+          ELFSymTy NewColdSym = FunctionSymbol;
+          const SmallString<256> SymbolName = formatv(
+              "{0}.cold.{1}", cantFail(FunctionSymbol.getName(StringSection)),
+              FF.getFragmentNum().get() - 1);
+          NewColdSym.st_name = AddToStrTab(SymbolName);
+          NewColdSym.st_shndx =
----------------
Can we add a simple test to check that this is working as intended?

Mistakes in the symbol table update are nasty because the program will not crash and we may take a long time to detect (since it takes a human to read the symbol table and see that it doesn't make sense). For this case, a test is a good way of making sure this code won't break in the future.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:4623-4628
+        if (FF == Function->getLayout().fragment_end()) {
+          errs() << formatv("BOLT-WARNING: unable to find fragment containg "
+                            "secondary entrypoint of {0}\n",
+                            *Function);
+          FF = Function->getLayout().fragment_begin();
+        }
----------------
This needs to be aligned with getBinaryFunctionContainingAddress. If this function returned that this address is part of this function, but we can't find it in any of its fragments, then it is an assertion.  At this point in the stack, is getBinaryFunctionContainingAddress already considering fragments? (I would imagine so)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132051