[PATCH] D85368: [llvm][CodeGen] Machine Function Splitter

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:111
+  // Commonly this will be empty and will only show up for section names
+  // specified by __attribute__((section(name)).
+  StringRef SectionName = MF.getFunction().getSection();
----------------
tmsriram wrote:
> I think excluding sections needs a bit more thought and we should do this as a separate patch if it is useful but I think a linker solution would be more favorable.
> 
> 1) From what I understand, when a user specifies section names using the section keyword, then the expectation is that all functions marked with that section name will be grouped together.  With function splitting, since you attach the ".cold" suffix to such sections that are split, there is no guarantee that the linker will place them together as these are not prefixed as ".text".   
> 
> 2) To overcome the above problem, the option to exclude such sections from being split is not ideal either as it moves the burden to the user to get this right with appropriate options.
> 
> 3) I think the temporary fix is to not split sections which are not prefixed as ".text".  You can add a "FIXME:" comment here to describe why you are doing this.  Moving forward, we can look at a linker solution where '.' is treated as a valid section name separator and sections with identical prefix before the "." are always grouped together even if they are not named ".text". 
> 
> 4) I think we can move this handling as an enhancement in another patch.  
> 
Correction: I meant ".unlikely" and not ".cold".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85368