[llvm] Extend the mismatch attr check to all non-exact definition function (PR #88035)
Lei Wang via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 9 13:57:10 PDT 2024
https://github.com/wlei-llvm updated https://github.com/llvm/llvm-project/pull/88035
>From b1b45a7a2f1f545c0d7112c29702c9bc6153969f Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Mon, 8 Apr 2024 11:33:36 -0700
Subject: [PATCH 1/4] Extend the mismatch attr check to all MayBeDerefinf
function
---
.../Utils/SampleProfileLoaderBaseImpl.h | 33 +++++++++----------
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index 581d354fc4766c..afc91170f9b9f7 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -129,25 +129,24 @@ class PseudoProbeManager {
bool profileIsValid(const Function &F, const FunctionSamples &Samples) const {
const auto *Desc = getDesc(F);
- bool IsAvailableExternallyLinkage =
- GlobalValue::isAvailableExternallyLinkage(F.getLinkage());
+ bool MayBeDerefined = !F.isDefinitionExact();
// Always check the function attribute to determine checksum mismatch for
- // `available_externally` functions even if their desc are available. This
- // is because the desc is computed based on the original internal function
- // and it's substituted by the `available_externally` function during link
- // time. However, when unstable IR or ODR violation issue occurs, the
- // definitions of the same function across different translation units could
- // be different and result in different checksums. So we should use the
- // state from the new (available_externally) function, which is saved in its
- // attribute.
- assert((LTOPhase != ThinOrFullLTOPhase::ThinLTOPostLink ||
- IsAvailableExternallyLinkage || !Desc ||
- profileIsHashMismatched(*Desc, Samples) ==
- F.hasFnAttribute("profile-checksum-mismatch")) &&
- "In post-link, profile checksum matching state doesn't match the "
- "internal function's 'profile-checksum-mismatch' attribute.");
+ // functions whose definition is not exact(MayBeDerefined) even if their
+ // desc are available. This is because the desc is computed based on the
+ // original internal definition. When a derefined case occurs, the
+ // definition of function may be replaced by a differently optimized variant
+ // of the same source level function at link time and this could result in
+ // different checksums. So we should use the state from the new definition,
+ // which is saved in its attribute.
+ assert(
+ (LTOPhase != ThinOrFullLTOPhase::ThinLTOPostLink || MayBeDerefined ||
+ !Desc ||
+ profileIsHashMismatched(*Desc, Samples) ==
+ F.hasFnAttribute("profile-checksum-mismatch")) &&
+ "In post-link, profile checksum matching state doesn't match the exact "
+ "definition function's 'profile-checksum-mismatch' attribute.");
(void)LTOPhase;
- if (IsAvailableExternallyLinkage || !Desc)
+ if (MayBeDerefined || !Desc)
return !F.hasFnAttribute("profile-checksum-mismatch");
return Desc && !profileIsHashMismatched(*Desc, Samples);
>From 637addb2acdad14864552161e4f6a65ef6bf86f8 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Mon, 8 Apr 2024 22:17:09 -0700
Subject: [PATCH 2/4] Update variable name and comment
---
.../Utils/SampleProfileLoaderBaseImpl.h | 21 +++++++++----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index afc91170f9b9f7..c095ffca312e8d 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -129,24 +129,23 @@ class PseudoProbeManager {
bool profileIsValid(const Function &F, const FunctionSamples &Samples) const {
const auto *Desc = getDesc(F);
- bool MayBeDerefined = !F.isDefinitionExact();
+ bool NonExactDefinition = !F.isDefinitionExact();
// Always check the function attribute to determine checksum mismatch for
- // functions whose definition is not exact(MayBeDerefined) even if their
- // desc are available. This is because the desc is computed based on the
- // original internal definition. When a derefined case occurs, the
- // definition of function may be replaced by a differently optimized variant
- // of the same source level function at link time and this could result in
- // different checksums. So we should use the state from the new definition,
- // which is saved in its attribute.
+ // functions whose definition is not exact even if their desc are available.
+ // This is because the desc is computed based on the original internal
+ // definition, however, the definition may be replaced by a different
+ // optimized variant of the same source level function at link time and this
+ // could result in different checksums. So we should use the state from the
+ // new definition, which is saved in its attribute.
assert(
- (LTOPhase != ThinOrFullLTOPhase::ThinLTOPostLink || MayBeDerefined ||
- !Desc ||
+ (LTOPhase != ThinOrFullLTOPhase::ThinLTOPostLink ||
+ NonExactDefinition || !Desc ||
profileIsHashMismatched(*Desc, Samples) ==
F.hasFnAttribute("profile-checksum-mismatch")) &&
"In post-link, profile checksum matching state doesn't match the exact "
"definition function's 'profile-checksum-mismatch' attribute.");
(void)LTOPhase;
- if (MayBeDerefined || !Desc)
+ if (NonExactDefinition || !Desc)
return !F.hasFnAttribute("profile-checksum-mismatch");
return Desc && !profileIsHashMismatched(*Desc, Samples);
>From 57d84ee191faa58bf915129f6a14602856e7572c Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Mon, 8 Apr 2024 23:57:03 -0700
Subject: [PATCH 3/4] move the check to post-link only
---
.../llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index c095ffca312e8d..b7d4c356ff676d 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -144,8 +144,8 @@ class PseudoProbeManager {
F.hasFnAttribute("profile-checksum-mismatch")) &&
"In post-link, profile checksum matching state doesn't match the exact "
"definition function's 'profile-checksum-mismatch' attribute.");
- (void)LTOPhase;
- if (NonExactDefinition || !Desc)
+ if (LTOPhase == ThinOrFullLTOPhase::ThinLTOPostLink &&
+ (NonExactDefinition || !Desc))
return !F.hasFnAttribute("profile-checksum-mismatch");
return Desc && !profileIsHashMismatched(*Desc, Samples);
>From b0eef00048e1fee35bc039b9ae5423130dd84ae8 Mon Sep 17 00:00:00 2001
From: wlei <wlei at fb.com>
Date: Tue, 9 Apr 2024 13:56:05 -0700
Subject: [PATCH 4/4] Remove the assertion to unblock breakages
---
.../Utils/SampleProfileLoaderBaseImpl.h | 27 ++++++++-----------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
index b7d4c356ff676d..b3e3833040aaec 100644
--- a/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
@@ -129,23 +129,18 @@ class PseudoProbeManager {
bool profileIsValid(const Function &F, const FunctionSamples &Samples) const {
const auto *Desc = getDesc(F);
- bool NonExactDefinition = !F.isDefinitionExact();
+ bool IsAvailableExternallyLinkage =
+ GlobalValue::isAvailableExternallyLinkage(F.getLinkage());
// Always check the function attribute to determine checksum mismatch for
- // functions whose definition is not exact even if their desc are available.
- // This is because the desc is computed based on the original internal
- // definition, however, the definition may be replaced by a different
- // optimized variant of the same source level function at link time and this
- // could result in different checksums. So we should use the state from the
- // new definition, which is saved in its attribute.
- assert(
- (LTOPhase != ThinOrFullLTOPhase::ThinLTOPostLink ||
- NonExactDefinition || !Desc ||
- profileIsHashMismatched(*Desc, Samples) ==
- F.hasFnAttribute("profile-checksum-mismatch")) &&
- "In post-link, profile checksum matching state doesn't match the exact "
- "definition function's 'profile-checksum-mismatch' attribute.");
- if (LTOPhase == ThinOrFullLTOPhase::ThinLTOPostLink &&
- (NonExactDefinition || !Desc))
+ // `available_externally` functions even if their desc are available. This
+ // is because the desc is computed based on the original internal function
+ // and it's substituted by the `available_externally` function during link
+ // time. However, when unstable IR or ODR violation issue occurs, the
+ // definitions of the same function across different translation units could
+ // be different and result in different checksums. So we should use the
+ // state from the new (available_externally) function, which is saved in its
+ // attribute.
+ if (IsAvailableExternallyLinkage || !Desc)
return !F.hasFnAttribute("profile-checksum-mismatch");
return Desc && !profileIsHashMismatched(*Desc, Samples);
More information about the llvm-commits
mailing list