Add dependency on ProfileData from ScalarOpts
#153651
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms Author: Mircea Trofin (mtrofin) ChangesFixing buildbot failures after PR #153305, e.g. https://lab.llvm.org/buildbot/#/builders/203/builds/19861 Analysis already depends on Also avoided an extra dependency (and very unnecessary) on Full diff: https://github.com/llvm/llvm-project/pull/153651.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/CMakeLists.txt b/llvm/lib/Transforms/Scalar/CMakeLists.txt
index 84a5b02043d01..765059d0c3b20 100644
--- a/llvm/lib/Transforms/Scalar/CMakeLists.txt
+++ b/llvm/lib/Transforms/Scalar/CMakeLists.txt
@@ -95,6 +95,7 @@ add_llvm_component_library(LLVMScalarOpts
Analysis
Core
InstCombine
+ ProfileData
Support
TransformUtils
)
diff --git a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
index 6719ce64b96b6..206bcb1080507 100644
--- a/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
+++ b/llvm/lib/Transforms/Scalar/JumpTableToSwitch.cpp
@@ -22,7 +22,6 @@
#include "llvm/ProfileData/InstrProf.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Error.h"
-#include "llvm/Transforms/Instrumentation/PGOInstrumentation.h"
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include <limits>
@@ -182,8 +181,14 @@ expandToSwitch(CallBase *CB, const JumpTableTy &JT, DomTreeUpdater &DTU,
if (HadProfile && !ProfcheckDisableMetadataFixes) {
// At least one of the targets must've been taken.
assert(llvm::any_of(BranchWeights, [](uint64_t V) { return V != 0; }));
- setProfMetadata(F.getParent(), Switch, BranchWeights,
- *llvm::max_element(BranchWeights));
+ // FIXME: this duplicates logic in instrumentation. Note: since there's at
+ // least a nonzero and these are unsigned values, it follows MaxBW != 0.
+ uint64_t MaxBW = *llvm::max_element(BranchWeights);
+ SmallVector<uint32_t> ScaledBranchWeights(
+ llvm::map_range(BranchWeights, [MaxBW](uint64_t V) {
+ return static_cast<uint32_t>(V / MaxBW);
+ }));
+ setBranchWeights(*Switch, ScaledBranchWeights, /*IsExpected=*/false);
} else
setExplicitlyUnknownBranchWeights(*Switch);
if (PHI)
|
…edata_from_scalaropts
mtrofin
deleted the
users/mtrofin/08-14-add_dependency_on_profiledata_from_scalaropts
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/22623 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/21581 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/11592 Here is the relevant piece of the build log for the reference |
mtrofin
added a commit
that referenced
this pull request
Aug 15, 2025
This also fixes a bug introduced accidentally in #153651, whereby the `JumpTableToSwitch` would convert all the branch weights to 0 except for one. It didn't trip the test because `update_test_checks` wasn't run with `-check-globals`. It is now. This also made noticeable that the direct calls promoted from the indirect call inherited the `VP`metadata, which should be dropped as it makes no more sense now.
mdenson
pushed a commit
to mdenson/llvm-project
that referenced
this pull request
Aug 16, 2025
Fixing buildbot failures after PR llvm#153305, e.g. https://lab.llvm.org/buildbot/#/builders/203/builds/19861 Analysis already depends on `ProfileData`, so the transitive closure of the dependencies of `ScalarOpts` doesn't change. Also avoided an extra dependency (and very unnecessary) on `Instrumentation`. The API previously used doesn't need to live in Instrumentation to begin with, but that's something to address in a follow-up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixing buildbot failures after PR #153305, e.g. https://lab.llvm.org/buildbot/#/builders/203/builds/19861
Analysis already depends on
ProfileData, so the transitive closure of the dependencies ofScalarOptsdoesn't change.Also avoided an extra dependency (and very unnecessary) on
Instrumentation. The API previously used doesn't need to live in Instrumentation to begin with, but that's something to address in a follow-up.