Skip to content

Deprecate order file instrumentation #121514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 3, 2025
Merged

Deprecate order file instrumentation #121514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
432c557
Deprecate order file instrumentation
ellishg Jan 2, 2025
06efe3d
format
ellishg Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1888,7 +1888,7 @@ defm pseudo_probe_for_profiling : BoolFOption<"pseudo-probe-for-profiling",
" pseudo probes for sample profiling">>;
def forder_file_instrumentation : Flag<["-"], "forder-file-instrumentation">,
Group<f_Group>, Visibility<[ClangOption, CC1Option, CLOption]>,
HelpText<"Generate instrumented code to collect order file into default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env var)">;
HelpText<"Generate instrumented code to collect order file into default.profraw file (overridden by '=' form of option or LLVM_PROFILE_FILE env var). Deprecated, please use temporal profiling.">;
def fprofile_list_EQ : Joined<["-"], "fprofile-list=">,
Group<f_Group>, Visibility<[ClangOption, CC1Option, CLOption]>,
HelpText<"Filename defining the list of functions/files to instrument. "
Expand Down
22 changes: 13 additions & 9 deletions clang/lib/Driver/ToolChains/Clang.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8010,15 +8010,19 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
}
}

if (Args.hasArg(options::OPT_forder_file_instrumentation)) {
CmdArgs.push_back("-forder-file-instrumentation");
// Enable order file instrumentation when ThinLTO is not on. When ThinLTO is
// on, we need to pass these flags as linker flags and that will be handled
// outside of the compiler.
if (!IsUsingLTO) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-enable-order-file-instrumentation");
}
if (const Arg *A =
Args.getLastArg(options::OPT_forder_file_instrumentation)) {
D.Diag(diag::warn_drv_deprecated_arg)
<< A->getAsString(Args) << /*hasReplacement=*/true
<< "-mllvm -pgo-temporal-instrumentation";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unfortunate that the replacement for a clang -f option is a -mllvm flag. The -mllvm flags don't get documented in the help or manpage.

Should this instead be surfaced as a new flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These flags are extensively documented in this RFC and this EuroLLVM talk. And LLVM options do have descriptions like clang frontend flags. I think it might be overkill to turn this into a frontend flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFCs and developer meeting talks are not user documentation. The -mllvm flags are not documented in Clang’s -help, or manual (https://clang.llvm.org/docs/UsersManual.html).

As someone who was looking for this flag last week, I don’t believe we should be deprecating a documented publicly facing option with the replacement being undocumented.

cc: @AaronBallman & @MaskRay

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this shows the importance of deprecating this flag as using IRPGO has better support and will give better performance gains. I wasn't sure how many people are using this flag, so I'm glad you reached out.

I can add a section to describe how to use this flag in the user manual (https://clang.llvm.org/docs/UsersManual.html#profiling-with-instrumentation). Would that be sufficient?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be a -f flag so that it gets documented in -help, the manpage and the UsersManual web page.

Copy link
Member

@MaskRay MaskRay Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it looks kinda unusual, I think it's fine in this case.

I believe -forder-file-instrumentation, proposed by Meta (Facebook), was only used by Meta. Now there is a replacement and -forder-file-instrumentation is no longer needed. This deprecation message is more for courtesy, but I dough anyone will notice it. Temporal profiler authors probably don't want to expose -mllvm xxx as a driver option yet.

I think that a lot of instrumentation options where the authors don't want to promise too much stability for certain niche optional features. For this option Ellis has done more than absolutely required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-forder-file-instrumentation was not only used by Meta. At a minimum I’ve been using it, and my former team at Apple was too. The instrumentation-based approaches to order file generation are significantly more robust than the dtrace-based solution (https://github.com/llvm/llvm-project/blob/main/clang/utils/perf-training/perf-helper.py#L128), that I upstreamed from Apple’s internal tooling in 2016 (d8b5bde).

I don’t know if Apple ever updated their Clang builds to use the instrumentation-based approach, but it is vastly superior. Notably dtrace is not deterministic, so using it to drive part of a build process is not ideal.

I worry about having a deprecation policy allowing deprecating a publicly documented option for a non-public option. If the new feature isn’t stable, and the old feature still works, why are we deprecating it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new clang option with good documentation can be useful. How about something like -ffunction-timing-profile?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-pgo-temporal-instrumentation is stable and a significant improvement over -forder-file-instrumentation. I'm happy to help answer any questions about migrating to IRPGO, and I don't mind delaying the removal if necessary.

I suppose I could add a frontend flag similar to how #109837 added -fprofile-generate-cold-function-coverage. I'll think about the naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the flag in #122385

CmdArgs.push_back("-forder-file-instrumentation");
// Enable order file instrumentation when ThinLTO is not on. When ThinLTO is
// on, we need to pass these flags as linker flags and that will be handled
// outside of the compiler.
if (!IsUsingLTO) {
CmdArgs.push_back("-mllvm");
CmdArgs.push_back("-enable-order-file-instrumentation");
}
}

if (Arg *A = Args.getLastArg(options::OPT_fforce_enable_int128,
Expand Down
2 changes: 2 additions & 0 deletions clang/test/Driver/clang_f_opts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@
// RUN: -fno-devirtualize-speculatively \
// RUN: -fslp-vectorize-aggressive \
// RUN: -fno-slp-vectorize-aggressive \
// RUN: -forder-file-instrumentation \
// RUN: %s 2>&1 | FileCheck --check-prefix=CHECK-WARNING %s
// CHECK-WARNING-DAG: optimization flag '-finline-limit=1000' is not supported
// CHECK-WARNING-DAG: optimization flag '-finline-limit' is not supported
Expand Down Expand Up @@ -423,6 +424,7 @@
// CHECK-WARNING-DAG: optimization flag '-fno-devirtualize-speculatively' is not supported
// CHECK-WARNING-DAG: the flag '-fslp-vectorize-aggressive' has been deprecated and will be ignored
// CHECK-WARNING-DAG: the flag '-fno-slp-vectorize-aggressive' has been deprecated and will be ignored
// CHECK-WARNING-DAG: argument '-forder-file-instrumentation' is deprecated, use '-mllvm -pgo-temporal-instrumentation' instead

// Test that we mute the warning on these
// RUN: %clang -### -finline-limit=1000 -Wno-invalid-command-line-argument \
Expand Down
Loading