[clang] Add command line options for the Machine Function Splitter.

[snehasish]

Summary

This patch adds a command line flag for the machine function splitter
(added in rG94faadaca4e1). The command line reference has been updated
with the new flag and tests added to check the pass is run (requires
profile data) and that the driver has registered the correct option. A
warning is emitted if the option is enabled when no profile is provided.

-fsplit-machine-functions
Split machine functions using profile information (x86-elf only). On
other targets an error is emitted. If profile information is not
provided a warning is emitted notifying the user of backend
optimizations failure.

[davidxl]

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

Also is it worth to give a warning if the option is specified but PGO is not on?

[snehasish]

Add warning when option is enabled without profile.

[snehasish]

Thanks for the comments! PTAL.

In D87047#2252982, @davidxl wrote:

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

I don't think we are there yet to turn it on for x86 profile builds. There is at least one unresolved issue with DebugInfo (D85085); once we have tested this some more internally we can revisit and make it the default.

Also is it worth to give a warning if the option is specified but PGO is not on?

Added a warning if no profile is specified but this option is turned on.

[tmsriram]

This LGTM.

[snehasish]

Add a check for x86-elf.

• Add a check for the triple in the driver to only enable the flag on x86+elf targets.
• Add a test to ensure we don't enable this on other targets.

[snehasish]

Updated test and warning type.

• Updated test to check for the diagnostic message.
• Updated BackendUtil.cpp to use backend optimization failure warning.

[snehasish]

Check warning, specify target to avoid failures on windows.

• Added a check for warning emitted if no profile is provided.
• Added a triple for the remaining checks since we now emit an error for incompatible targets.

[MaskRay]

clang/include/clang/Driver/Options.td

1992

Please use OptInFFlag and see its comment.

clang/lib/Driver/ToolChains/Clang.cpp

4259

This is not needed.

This is for fembed-bitcode and people seem to randomly add options here. Many options are probably not needed.

4917

If you use getLastArg

A->getAsString(Args)

clang/test/CodeGen/split-machine-functions.c

4

Consider RUN: split-file

Search for this string for some examples.

[snehasish]

Use OptInFFlag, split-file and update tests.

• Change the flag type to OptInFFlag.
• Use split-file in the test to avoid "RUN: echo" lines.
• Use an existing warn message (if no profile is available) and add a check for it.

[snehasish]

PTAL, thanks!

clang/lib/Driver/ToolChains/Clang.cpp

4259

Thanks for catching this. I read this list as "options which should be ignored for embedding bitcode". In this case we do want to save this flag to pass back to clang if necessary (as mtrofin@ pointed out offline). Perhaps the documentation around this could be enhanced.

clang/test/CodeGen/split-machine-functions.c

4

Looks much cleaner with split-file overall, though it doesn't play well with clang-format since this is a .c file. Should we just ignore clang-format complaints for this file?

[snehasish]

Fix test formatting.

Added clang-format off to disable clang format for the test which contains both profile data (as text) and c code.

[snehasish]

clang/test/CodeGen/split-machine-functions.c

4

Fixed the formatting by adding a clang-format off directive at the top of this file.

[MaskRay]

clang/docs/ClangCommandLineReference.rst

2131

You don't need to edit this file. It is auto-generated from Options.td

clang/lib/Driver/ToolChains/Clang.cpp

4914

OPT_fno_split_machine_functions is a driver option but not a CC1 option. Rendering OPT_fno_split_machine_functions will error when processing CC1 options.

clang/test/CodeGen/split-machine-functions.c

2

You can ignore clang-format/clang-tidy issues in test/ and there is no need to annotate the tests (otherwise we will annotate essentially every file)

6

See https://reviews.llvm.org/D85408#2266559

It feels wrong that the assembly+llvm-profdata test is in clang/test

clang/test/Driver/fsplit-machine-functions.c

2

Add -fprofile-use to the canonical usage line

4

Two -c?

[snehasish]

Remove clang/CodeGen test, update arg render logic.

• Removed the clang/CodeGen test.
• Added a check for the option to be rendered.
• Fixed extra flags in driver test.

[snehasish]

It feels wrong that the assembly+llvm-profdata test is in clang/test

I agree with @dblaikie and your assessment that it feels wrong to add such a test to clang. In the first version of this patch, the test served the purpose of representing the canonical usage with profdata as well as check for the plumbing of the profile along with the presence of the flag. Enhancing the driver test removed the need for the former while adding a diagnostic warning message (and a check for it) removes the need for the latter. I've removed the clang codegen test.

Thanks for the detailed review, PTAL.

[snehasish]

Rebased patch.

[snehasish]

@MaskRay ping, let me know if you have any further comments. Thanks!

[MaskRay]

clang/test/Driver/fsplit-machine-functions.c

4

There are two -c on this line.

Testing warnings can use -### as well.

[snehasish]

Remove extra -c from test command line.

[snehasish]

clang/test/Driver/fsplit-machine-functions.c

4

Removed the extra -c.
The warning is emitted when CreateTargetMachine (and its children) are called. So using -### instead of -c for the CHECK-WARN test is not sufficient as far as I can tell.

[MaskRay]

clang/test/Driver/fsplit-machine-functions.c

4

If you make a compile action, this may need x86-registered-target.

[MaskRay]

clang/lib/CodeGen/BackendUtil.cpp

517

I did not pay attention. Such compatibility checks should be added in lib/Driver, instead of lib/CodeGen.

clang/test/Driver/fsplit-machine-functions.c

4

I did not pay attention. If you added checks to lib/Driver, x86-registered-target would not be needed and you could use -###

[snehasish]

Check profile flag in Driver, update test.

• Move the profile missing warning check to the Driver.
• Update the test to use -### for CHECK-WARN.

[snehasish]

That makes sense. I moved the check to lib/Driver/ToolChains/Clang.cpp and updated the test. Seems cleaner to have all the checks in one place.
PTAL, thanks.

[MaskRay]

clang/include/clang/Driver/Options.td

1993

If this can be adapted to other targets, there is no need to specifically mention x86-elf only ("x86 ELF")

clang/lib/CodeGen/BackendUtil.cpp

12

This is not needed

clang/lib/Driver/ToolChains/Clang.cpp

27

Not needed?

[snehasish]

Remove unnecessary includes, update doc text.

• Update doctext.
• Remove no longer necessary includes to frontend driver diagnostics.

[MaskRay]

clang/include/clang/Driver/Options.td

1993

So I take quickly a look at rG94faadaca4e1704f674d2e9d4a1d25643b9ca52c. There is nothing ELF specific, right? At least I don't expect AArch64 ELF to fail. If you do think this merits a "ELF only" comment, adding it seems feasible.

clang/test/Driver/fsplit-machine-functions.c

9

For diagnostics, we usually add the warning or error prefix.

[snehasish]

Update the test.

• Added "warning: " prefix to be checked.

[snehasish]

Thanks for the review.

clang/include/clang/Driver/Options.td

1993

The machine function splitter uses the basic block sections feature. This has not been tested on AArch64, there is some target specific code in the CFI handling as well idiosyncrasies of each target which need to be worked out. I'm going to leave this as it is since the checks included in this patch enforce x86 & ELF. This is on our longer term roadmap and we will revisit this in the future.