[Driver] Remove ignored Flag form of -fauto-profile/-fprofile-sample-use

[MaskRay]

The Flag form options are accepted and silently ignored, which can be
surprising. The Eq form is supposed to be used instead, e.g.
-fprofile-sample-use=a.afdo.

Since we does not intend to support GCC's "fbaata.afdo" filename, just
remove the two options. While here, clean up code as -fauto-profile= is
an alias.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Fangrui Song (MaskRay)

Changes

The Flag form options are accepted and silently ignored, which can be
surprising. The Eq form is supposed to be used instead, e.g.
-fprofile-sample-use=a.prof.

Since we does not intend to support GCC's "fbaata.afdo" filename, just
remove the two options. While here, clean up code as -fauto-profile= is
an alias.

---

Full diff: https://github.com/llvm/llvm-project/pull/113528.diff

4 Files Affected:

• (modified) clang/include/clang/Driver/Options.td (-4)
• (modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+5-10)
• (modified) clang/test/Driver/clang_f_opts.c (-3)
• (added) clang/test/Driver/fprofile-sample-use.c (+5)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 2ddb2f5312148e..67a4b69b747767 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1729,8 +1729,6 @@ defm gnu_inline_asm : BoolFOption<"gnu-inline-asm",
           "Disable GNU style inline asm">,
   PosFlag<SetTrue>>;
 
-def fprofile_sample_use : Flag<["-"], "fprofile-sample-use">, Group<f_Group>,
-    Visibility<[ClangOption, CLOption]>;
 def fno_profile_sample_use : Flag<["-"], "fno-profile-sample-use">, Group<f_Group>,
     Visibility<[ClangOption, CLOption]>;
 def fprofile_sample_use_EQ : Joined<["-"], "fprofile-sample-use=">,
@@ -1756,8 +1754,6 @@ def fsample_profile_use_profi : Flag<["-"], "fsample-profile-use-profi">,
                basic block counts to branch probabilites to fix them by extended
                and re-engineered classic MCMF (min-cost max-flow) approach.}]>;
 def fno_profile_sample_accurate : Flag<["-"], "fno-profile-sample-accurate">, Group<f_Group>;
-def fauto_profile : Flag<["-"], "fauto-profile">, Group<f_Group>,
-    Alias<fprofile_sample_use>;
 def fno_auto_profile : Flag<["-"], "fno-auto-profile">, Group<f_Group>,
     Alias<fno_profile_sample_use>;
 def fauto_profile_EQ : Joined<["-"], "fauto-profile=">,
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 91605a67a37fc0..c4c14b2d6870c8 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -1758,18 +1758,13 @@ Arg *tools::getLastProfileUseArg(const ArgList &Args) {
 
 Arg *tools::getLastProfileSampleUseArg(const ArgList &Args) {
   auto *ProfileSampleUseArg = Args.getLastArg(
-      options::OPT_fprofile_sample_use, options::OPT_fprofile_sample_use_EQ,
-      options::OPT_fauto_profile, options::OPT_fauto_profile_EQ,
-      options::OPT_fno_profile_sample_use, options::OPT_fno_auto_profile);
-
-  if (ProfileSampleUseArg &&
-      (ProfileSampleUseArg->getOption().matches(
-           options::OPT_fno_profile_sample_use) ||
-       ProfileSampleUseArg->getOption().matches(options::OPT_fno_auto_profile)))
+      options::OPT_fprofile_sample_use_EQ, options::OPT_fno_profile_sample_use);
+
+  if (ProfileSampleUseArg && (ProfileSampleUseArg->getOption().matches(
+                                 options::OPT_fno_profile_sample_use)))
     return nullptr;
 
-  return Args.getLastArg(options::OPT_fprofile_sample_use_EQ,
-                         options::OPT_fauto_profile_EQ);
+  return Args.getLastArg(options::OPT_fprofile_sample_use_EQ);
 }
 
 const char *tools::RelocationModelName(llvm::Reloc::Model Model) {
diff --git a/clang/test/Driver/clang_f_opts.c b/clang/test/Driver/clang_f_opts.c
index fd15552715cb35..77e737db8158b4 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -71,9 +71,6 @@
 // RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-auto-profile %s 2>&1 | FileCheck -check-prefix=CHECK-NO-AUTO-PROFILE %s
 // CHECK-NO-AUTO-PROFILE-NOT: "-fprofile-sample-use={{.*}}/file.prof"
 
-// RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-profile-sample-use -fauto-profile %s 2>&1 | FileCheck -check-prefix=CHECK-AUTO-PROFILE %s
-// RUN: %clang -### -S -fauto-profile=%S/Inputs/file.prof -fno-auto-profile -fprofile-sample-use %s 2>&1 | FileCheck -check-prefix=CHECK-AUTO-PROFILE %s
-
 // RUN: %clang -### -S -fprofile-generate %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-LLVM %s
 // RUN: %clang -### -S -fprofile-instr-generate %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE %s
 // RUN: %clang -### -S -fprofile-generate=/some/dir %s 2>&1 | FileCheck -check-prefix=CHECK-PROFILE-GENERATE-DIR %s
diff --git a/clang/test/Driver/fprofile-sample-use.c b/clang/test/Driver/fprofile-sample-use.c
new file mode 100644
index 00000000000000..7c8813a83785ce
--- /dev/null
+++ b/clang/test/Driver/fprofile-sample-use.c
@@ -0,0 +1,5 @@
+/// GCC -fauto-profile (without =) is rejected.
+/// -fprofile-sample-use without = is rejected as well.
+// RUN: not %clang -### -S -fauto-profile -fprofile-sample-use %s 2>&1 | FileCheck %s --check-prefix=ERR
+// ERR: error: unknown argument: '-fauto-profile'
+// ERR: error: unknown argument: '-fprofile-sample-use'

[mikolaj-pirog]

This would close #112391

I am copying my comment from my PR #112750 which addresses the issue #112391:
I think this isn't the best solution, since it is inconsistent with the behavior of the -fprofile-use option. Why should -fprofile-use be accepted and -fprofile-sample-use rejected? TBH I think the whole looking for default filename isn't the best option, but to make things consistent it seems to easier to introduce this behavior for -fprofile-sample/auto-use rather than take it from -fprofile-use

[MaskRay]

This would close #112391

I am copying my comment from my PR #112750 which addresses the issue #112391: I think this isn't the best solution, since it is inconsistent with the behavior of the -fprofile-use option. Why should -fprofile-use be accepted and -fprofile-sample-use rejected? TBH I think the whole looking for default filename isn't the best option, but to make things consistent it seems to easier to introduce this behavior for -fprofile-sample/auto-use rather than take it from -fprofile-use

I know this is a bit inconsistent, and the original patch in 2017 probably should not have added -fauto-profile/-fauto-profile= in the first place (unneeded, incompatible with GCC anyway).

I think this is a little compatibility argument to retain -fprofile-use, but there isn't any for -fprofile-sample-use=. The usual filename is *.afdo *.xfdo, not .profdata (indexed profile format).

I'd just fix -fprofile-sample-use=.

[mikolaj-pirog]

It's true that there isn't any compatibility argument for leaving -fprofile-sample/auto (it was silently ignored afterall), but I think the consistency argument is important. From a standpoint of a clang user, it's very reasonable to expect that fprofile-use and -fprofile-sample-use will behave similarly, and inconsistent behavior leads to confusion.

[MaskRay]

It's true that there isn't any compatibility argument for leaving -fprofile-sample/auto (it was silently ignored afterall), but I think the consistency argument is important. From a standpoint of a clang user, it's very reasonable to expect that fprofile-use and -fprofile-sample-use will behave similarly, and inconsistent behavior leads to confusion.

I understand the option similarity argument, but I think it's outweighed by the drawback of having a misleading Flag form with a confusing file extension. With -fprofile-use=, people do use default.prof, but sample PGO uses very different extension names.

[mikolaj-pirog]

Regarding the default filename extension, it doesn't have to be .profdata for fprofile-sample-use, it can be the .afdo, or any other extension deemed proper. fprofile-sample-use and fprofile-use would behave similarly then: they would look for a default file with appropriate extension, which seems like a reasonable behavior to me

[MaskRay]

Regarding the default filename extension, it doesn't have to be .profdata for fprofile-sample-use, it can be the .afdo, or any other extension deemed proper. fprofile-sample-use and fprofile-use would behave similarly then: they would look for a default file with appropriate extension, which seems like a reasonable behavior to me

I respectively disagree. The sample PGO framework is quite different from instrumentation PGO. There is no -fprofile-sample-generate. While the naming -fprofile-sample-use= is similar to -fprofile-use=, it's a very weak argument to have a default filename, when the filename in the wild has many uses of very different extensions.

The deployment experience has shown that we don't really need a default filename.

[HaohaiWen]

Why don't also remove -fno-profile-sample-use and just leave -fprofile-sample-use=.
Is there any scenario to use -fno-profile-sample-use?

[MaskRay]

If -fprofile-sample-use= is added as a default option, -fno-profile-sample-use= can be used to cancel it. For example, it could be used as a debugging tool when you think one TU triggers a compiler bug.

[mikolaj-pirog]

Regarding the default filename extension, it doesn't have to be .profdata for fprofile-sample-use, it can be the .afdo, or any other extension deemed proper. fprofile-sample-use and fprofile-use would behave similarly then: they would look for a default file with appropriate extension, which seems like a reasonable behavior to me

I respectively disagree. The sample PGO framework is quite different from instrumentation PGO. There is no -fprofile-sample-generate. While the naming -fprofile-sample-use= is similar to -fprofile-use=, it's a very weak argument to have a default filename, when the filename in the wild has many uses of very different extensions.

The deployment experience has shown that we don't really need a default filename.

I see your reasoning and overall agree that approach of removing the flag option altogether is sensible

[MaskRay]

Ping:)