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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish marked an inline comment as done.
snehasish added a comment.

Thanks for the reviews! 
@hiraditya I've added a test to ensure ehpads are not split out. Let me know if there are additional cases you want to cover.



================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:232
+assignSections(MachineFunction &MF,
+               const std::vector<Optional<BBClusterInfo>> &FuncBBClusterInfo) {
   assert(MF.hasBBSections() && "BB Sections is not set for function.");
----------------
hiraditya wrote:
> nit: tab?
This seems to be the output from clang-format. The diff does not have a tab but it looks like phabricator is showing it as one?


================
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();
----------------
efriedma wrote:
> tmsriram wrote:
> > 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".
> I think I'd rather make splitting for functions with an explicit section attribute opt-in, rather than opt-out.  The user might have a strong need to emit a function in a particular section (for example, if the name is mentioned in a linker script).  If someone is messing with section attributes in the first place, I'd like to be conservative by default.
@tmsriram I think you meant to exclude such functions from being split. I agree, taking into consideration @efriedma's comment, I've removed the option and made this a conservative check on the section attribute. Any function with the section attribute set is not split.


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:113
+  StringRef SectionName = MF.getFunction().getSection();
+  for (const auto &ES : ExcludedSections) {
+    if (SectionName.equals(ES)) {
----------------
hiraditya wrote:
> std::find?
Code referencing this comment was removed.


================
Comment at: llvm/lib/CodeGen/TargetPassConfig.cpp:218
+static cl::opt<bool> EnableMachineFunctionSplitter(
+    "enable-split-machine-functions", cl::Hidden,
+    cl::desc("Split out cold blocks from machine functions based on profile "
----------------
efriedma wrote:
> snehasish wrote:
> > tmsriram wrote:
> > > Why not call this split-machine-functions too for consistency?
> > We can't register two options with the same string, i.e. "split-machine-functions". 
> Why do we need two options to control the same thing?
In this patch we added two options
1. An option in llvm/lib/CodeGen/CommandFlags.cpp "split-machine-functions" so that llc can be used to invoke it in the tests.
2. We added a temporary option in llvm/lib/CodeGen/TargetPassConfig.cpp so that it can be invoked when running with clang or lld (for LTO).

AFAICT we cant use (2) for tests and having (1) makes it easy to compile things without an intermediate llc step. We plan on removing (2) in a future patch which will add appropriate options to clang (-fsplit-machine-functions) and lld (--lto-split-machine-functions).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85368