Remove the assertion to unblock breakages

[wlei-llvm]

as titled.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Lei Wang (wlei-llvm)

Changes

Follow-up fix for #87279 which is not enough, we should apply the mismatch check to all cases that the definition can be replaced by another variant during link time. There is an existing API isDefinitionExact exactly used for this purpose, refer to the comments(https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/IR/GlobalValue.h#L134) from mayBeDerefined.
So we use this to extend the mismatch attr check.

---

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

1 Files Affected:

• (modified) llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h (+16-17)

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);

[davidtgoldblatt]

Looks good in general; 1 nit before merging: should be "redfined", not "derefined".

[wlei-llvm]

Looks good in general; 1 nit before merging: should be "redfined", not "derefined".

I see. Here both refinement and de-refinement can lead to a different version of IR (the checksum mismatch) in link time. How about just use "NonExactDefinition"?
(I thought the opposite of the isDefinitionExact was Derefined while I was looking at the code..)

  bool isDefinitionExact() const {
    return !mayBeDerefined();
  }

[davidtgoldblatt]

Ah, I see -- I think the original did was fine (and I just needed to learn more instead of assuming a typo).

[wlei-llvm]

Hmm, tested on our internal services, it still ran into the assertion.. the function linkage type is internal, it's very weird. How would an internal function be replaced by another definition.

[wlei-llvm]

Hmm, tested on our internal services, it still ran into the assertion.. the function linkage type is internal, it's very weird. How would an internal function be replaced by another definition.

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).
but in post-link time, profile is loaded(likely from other func's children profile), then find the state is mismatch.

This is probably not easy to quickly fix.. (Seems we need a three state for the attr: matched, mismatch, unknown..., maybe we don't need those complexity or just remove the assertion)

cc @WenleiHe

[wlei-llvm]

Let me remove the assertion to unblock the build, will dig more later.

[WenleiHe]

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).

Sounds good to remove assertion to unblock.

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).

I'm curious why does this happen?

[wlei-llvm]

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).

I'm curious why does this happen?

This can happen when meet:

1. there is no outline/top-level function in the profile( the function is all inlined into its caller or pre-inliner all inlines them).
2. because we use flattened profile. In prelink, all the internal function's nested(children) profile doesn't have this function.
3. In post-link, one (external) function 's nested profile have this function.

basically this is caused by the profile flattening.

[ayermolo]

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).

I'm curious why does this happen?

This can happen when meet:

1. there is no outline/top-level function in the profile( the function is all inlined into its caller or pre-inliner all inlines them).
2. because we use flattened profile. In prelink, all the internal function's nested(children) profile doesn't have this function.
3. In post-link, one (external) function 's nested profile have this function.

basically this is caused by the profile flattening.

What should happen in that case?
Is the test warranted?

[wlei-llvm]

What should happen in that case? Is the test warranted?

Before we expect a consistency(the assertion) between the mismatch state computed in pre-link and attr used in post-link, but because of this "issue", we understand that attr is not always equal to the pre-link state. Now I think we may just live with this, for local symbol, just use the original state, attr is only used when original state is missing(imported function).

Sorry for my bad linker knowledge, I thought the inconsistency was from the replacement of definition.

[WenleiHe]

OK, so this is because there is no profile loaded for this function in pre-link time (so it never run this profileIsValid), the attr is not set(unknown).

I'm curious why does this happen?

This can happen when meet:

1. there is no outline/top-level function in the profile( the function is all inlined into its caller or pre-inliner all inlines them).
2. because we use flattened profile. In prelink, all the internal function's nested(children) profile doesn't have this function.
3. In post-link, one (external) function 's nested profile have this function.

basically this is caused by the profile flattening.

Ok, so the function's profile only exists as nested inlinee profile in a different module, so for pre-link compilation of that module, we don't have a profile to check for mismatch.

[WenleiHe]

LGTM to unblock, thanks.