Deprecate order file instrumentation #121514
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Ellis Hoag (ellishg) Changes
Full diff: https://github.com/llvm/llvm-project/pull/121514.diff 3 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index d922709db17786..7e419fb889488d 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -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. "
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index a020e00cd17392..4f1bf64e8cb7e8 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -8010,7 +8010,11 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
}
}
- if (Args.hasArg(options::OPT_forder_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";
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
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index ddbf1fd951c84e..2b72068eae1eeb 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -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
@@ -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 \
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
No I wasn't planning on it. I'd like to remove this code for maintainability reasons and to encourage others to use IRPGO instead |
| Args.getLastArg(options::OPT_forder_file_instrumentation)) { | ||
| D.Diag(diag::warn_drv_deprecated_arg) | ||
| << A->getAsString(Args) << /*hasReplacement=*/true | ||
| << "-mllvm -pgo-temporal-instrumentation"; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think this needs to be a -f flag so that it gets documented in -help, the manpage and the UsersManual web page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
-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?
There was a problem hiding this comment.
A new clang option with good documentation can be useful. How about something like -ffunction-timing-profile?
There was a problem hiding this comment.
-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.
There was a problem hiding this comment.
Added the flag in #122385
-forder-file-instrumentationis used to instrument a startup function order, but this feature is also in IRPGO as Temporal Profiling. As discussed in https://discourse.llvm.org/t/deprecate-forder-file-instrumentation-in-favor-of-temporal-profiling/83903, this PR deprecates this flag.