Skip to content

Conversation

@mikolaj-pirog
Copy link
Member

@mikolaj-pirog mikolaj-pirog commented Oct 17, 2024

As in title. This matches the behavior of -fprofile-use, which when used without a path to the file, looks for default.profdata file in the current/supplied dir. Fixes #112391

@github-actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Oct 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 17, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Mikołaj Piróg (mikolaj-pirog)

Changes

As in title. This matches the behavior of -fprofile-use, which when used without a path to the file, looks for default.profdata file in the current/supplied dir. Fixes #112391


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

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Clang.cpp (+21)
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 3fc39296f44281..2da8b604ebff62 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -594,6 +594,8 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
         << PGOGenerateArg->getSpelling() << ProfileGenerateArg->getSpelling();
 
   auto *ProfileUseArg = getLastProfileUseArg(Args);
+  auto *ProfileSampleUseArg = Args.getLastArg(
+      options::OPT_fprofile_sample_use, options::OPT_fprofile_sample_use_EQ);
 
   if (PGOGenerateArg && ProfileUseArg)
     D.Diag(diag::err_drv_argument_not_allowed_with)
@@ -677,6 +679,25 @@ static void addPGOAndCoverageFlags(const ToolChain &TC, Compilation &C,
     }
   }
 
+  if (ProfileSampleUseArg) {
+    if ((ProfileSampleUseArg->getOption().matches(
+             options::OPT_fprofile_sample_use) ||
+         ProfileSampleUseArg->getOption().matches(
+             options::OPT_fprofile_sample_use_EQ))) {
+      SmallString<128> Path(ProfileSampleUseArg->getNumValues() == 0
+                                ? ""
+                                : ProfileSampleUseArg->getValue());
+      if (Path.empty() || llvm::sys::fs::is_directory(Path))
+        llvm::sys::path::append(Path, "default.profdata");
+
+      if (!llvm::sys::fs::exists(Path))
+        D.Diag(diag::err_drv_no_such_file) << Path;
+
+      CmdArgs.push_back(
+          Args.MakeArgString(Twine("-fprofile-sample-use=") + Path));
+    }
+  }
+
   bool EmitCovNotes = Args.hasFlag(options::OPT_ftest_coverage,
                                    options::OPT_fno_test_coverage, false) ||
                       Args.hasArg(options::OPT_coverage);

@mikolaj-pirog mikolaj-pirog changed the title [Clang] [PGO] Provide default value for -fprofile-sample-use [Clang] Provide default value for -fprofile-sample-use, -fprofile-auto Oct 18, 2024
@mikolaj-pirog mikolaj-pirog marked this pull request as draft October 18, 2024 13:05
@mikolaj-pirog mikolaj-pirog marked this pull request as ready for review October 18, 2024 13:23
@mikolaj-pirog
Copy link
Member Author

Pinging @MaskRay for review, I can't add reviewers in the dedicated gh section yet

@MaskRay
Copy link
Member

MaskRay commented Oct 23, 2024

Actually I think we should drop the accepted and ignored Flag form of -fprofile-sample-use and -fprofile-auto, which is misleading at the least. I have a pending patch, which I will upload at night.

For sample PGO, the profile file extension is usually .afdo, but I don't think it makes sense to have a default filename.

@mikolaj-pirog
Copy link
Member Author

Actually I think we should drop the accepted and ignored Flag form of -fprofile-sample-use and -fprofile-auto, which is misleading at the least. I have a pending patch, which I will upload at night.

For sample PGO, the profile file extension is usually .afdo, but I don't think it makes sense to have a default filename.

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

@mikolaj-pirog
Copy link
Member Author

Closing as #113528 fixed the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Clang] -fprofile-sample-use option has not effect

3 participants