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

[Fabian Parzefall via Phabricator via llvm-commits]

FPar added a comment.

Thanks for the feedback @rafauler!



================
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;
----------------
rafauler wrote:
> Can you explain why was that added a bit better?
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) {
----------------
rafauler wrote:
> 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)
You are right. Splitting into more than 2 fragments in non-relocation mode requires more work beyond this. I would rather have that in another diff so for now this checks that the function is split only 2-ways.


================
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();
+        }
----------------
rafauler wrote:
> 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)
`getBinaryFunctionContainingAddress` considers the original address range of the function, so it is not aware of fragments at all. This is fine though, because at this point `Symbol.st_value` still has the original value.

I agree about the assert. This checks now explicitly that the secondary entrypoint can be found in any of the function's fragments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132051