This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by snehasish on Sep 2 2020, 11:25 AM.

Details

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.

Diff Detail

Event Timeline

snehasish created this revision.Sep 2 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 2 2020, 11:25 AM
snehasish requested review of this revision.Sep 2 2020, 11:25 AM

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 updated this revision to Diff 289805.Sep 3 2020, 1:51 PM

Add warning when option is enabled without profile.

Thanks for the comments! PTAL.

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 accepted this revision.Sep 9 2020, 9:13 AM

This LGTM.

This revision is now accepted and ready to land.Sep 9 2020, 9:13 AM
snehasish updated this revision to Diff 290852.Sep 9 2020, 5:34 PM

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.

Updated test and warning type.

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

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.
snehasish edited the summary of this revision. (Show Details)Sep 10 2020, 12:32 PM
snehasish added a reviewer: MaskRay.
snehasish edited the summary of this revision. (Show Details)
MaskRay requested changes to this revision.Sep 10 2020, 2:50 PM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
1999

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.

4920

If you use getLastArg

A->getAsString(Args)

clang/test/CodeGen/split-machine-functions.c
3 ↗(On Diff #291063)

Consider RUN: split-file

Search for this string for some examples.

This revision now requires changes to proceed.Sep 10 2020, 2:50 PM
snehasish updated this revision to Diff 291108.Sep 10 2020, 5:50 PM

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 marked 3 inline comments as done.Sep 10 2020, 5:58 PM

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
3 ↗(On Diff #291063)

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 updated this revision to Diff 291122.Sep 10 2020, 8:54 PM
snehasish marked an inline comment as done.

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 marked an inline comment as done.Sep 10 2020, 8:55 PM
snehasish added inline comments.
clang/test/CodeGen/split-machine-functions.c
3 ↗(On Diff #291063)

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

MaskRay added inline comments.Sep 10 2020, 10:19 PM
clang/docs/ClangCommandLineReference.rst
2131 ↗(On Diff #291122)

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

clang/lib/Driver/ToolChains/Clang.cpp
4917

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
1 ↗(On Diff #291122)

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)

5 ↗(On Diff #291122)

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?

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 marked 3 inline comments as done.Sep 11 2020, 11:34 AM
snehasish added a subscriber: dblaikie.

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 updated this revision to Diff 291728.Sep 14 2020, 4:26 PM

Rebased patch.

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

MaskRay accepted this revision.Sep 14 2020, 4:51 PM
MaskRay added inline comments.
clang/test/Driver/fsplit-machine-functions.c
4

There are two -c on this line.

Testing warnings can use -### as well.

This revision is now accepted and ready to land.Sep 14 2020, 4:51 PM
snehasish updated this revision to Diff 291758.Sep 14 2020, 7:11 PM

Remove extra -c from test command line.

snehasish marked an inline comment as done.Sep 14 2020, 7:17 PM
snehasish added inline comments.
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 requested changes to this revision.Sep 14 2020, 8:03 PM
MaskRay added inline comments.
clang/test/Driver/fsplit-machine-functions.c
4

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

This revision now requires changes to proceed.Sep 14 2020, 8:03 PM
MaskRay added inline comments.Sep 14 2020, 8:25 PM
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 updated this revision to Diff 291771.Sep 14 2020, 9:11 PM
snehasish marked an inline comment as done.

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 marked 3 inline comments as done.Sep 14 2020, 9:15 PM

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 added inline comments.Sep 14 2020, 9:26 PM
clang/include/clang/Driver/Options.td
2000

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 updated this revision to Diff 291775.Sep 14 2020, 9:35 PM

Remove unnecessary includes, update doc text.

  • Update doctext.
  • Remove no longer necessary includes to frontend driver diagnostics.
snehasish marked 3 inline comments as done.Sep 14 2020, 9:36 PM
MaskRay accepted this revision.Sep 14 2020, 10:49 PM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
2000

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
8

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

This revision is now accepted and ready to land.Sep 14 2020, 10:49 PM
snehasish marked 2 inline comments as done.

Update the test.

  • Added "warning: " prefix to be checked.

Thanks for the review.

clang/include/clang/Driver/Options.td
2000

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.