Repository navigation
fix(tensilelite): export LazySingleton::Instance so the Debug singleton is not duplicated per DSO #12465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
fix(tensilelite): export LazySingleton::Instance so the Debug singleton is not duplicated per DSO #12465
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,7 +36,14 @@ namespace TensileLite | |
| class LazySingleton | ||
| { | ||
| public: | ||
| static Class& Instance() | ||
| // TENSILELITEHOST_EXPORT is required, not decorative. `Instance()` is an | ||
| // implicitly-inline member of a header-only template, and both | ||
| // libhipblaslt and libtensilelite-host are built with hidden visibility | ||
| // plus VISIBILITY_INLINES_HIDDEN. Without an explicit default-visibility | ||
| // attribute each shared object gets its OWN copy of `instance`, so state | ||
| // written through the singleton in one library is invisible in the other | ||
| // -- silently, with no link error. See ROCM-31245. | ||
| static TENSILELITEHOST_EXPORT Class& Instance() | ||
|
Alex-Vasile marked this conversation as resolved.
Resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also from the AI:
|
||
| { | ||
| static Class instance; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // Copyright Advanced Micro Devices, Inc., or its affiliates. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| #include <Tensile/Debug.hpp> | ||
|
|
||
| // ROCM-31245 regression guard. | ||
| // | ||
| // TensileLite::Debug is a LazySingleton: `Instance()` is an implicitly-inline | ||
| // member of a header-only class template holding a function-local static. Both | ||
| // libtensilelite-host and its consumers (libhipblaslt, this test binary) are | ||
| // built with -fvisibility=hidden and -fvisibility-inlines-hidden. Unless | ||
| // `Instance()` carries an explicit default-visibility attribute, each link unit | ||
| // gets its own private copy of the static, and there is no diagnostic of any | ||
| // kind: no link error, no warning, no crash. | ||
| // | ||
| // That is exactly what happened when TensileLite was split out of libhipblaslt | ||
| // into its own shared object. libhipblaslt wrote the | ||
| // "skip GridBasedMatching/PredictionMatching" set into its copy of Debug; | ||
| // ExactLogicLibrary::findAllSolutions read libtensilelite-host's copy, saw an | ||
| // empty set, and fully traversed the expensive ranking libraries on every | ||
| // hipblasLtMatmulAlgoGetHeuristic call -- a 100x-200x host-side regression. | ||
| // | ||
| // This test binary is a separate link unit from libtensilelite-host, so it | ||
| // stands in for libhipblaslt: if the singleton can duplicate across a library | ||
| // boundary at all, it duplicates here too. | ||
| namespace | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this test pass even without the fix if we are doing a static build? Is there a way to enforce dynamic linking for it? |
||
| { | ||
| TEST(SingletonIdentity, DebugSingletonIsSharedAcrossLinkUnits) | ||
| { | ||
| const void* fromThisLinkUnit | ||
| = static_cast<const void*>(&TensileLite::Debug::Instance()); | ||
| const void* fromTensileLiteHost = TensileLite::debugInstanceAddress(); | ||
|
|
||
| EXPECT_EQ(fromThisLinkUnit, fromTensileLiteHost) | ||
| << "TensileLite::Debug has been duplicated across a link-unit boundary. " | ||
| "Check that LazySingleton::Instance() still carries TENSILELITEHOST_EXPORT " | ||
| "(Tensile/Singleton.hpp). See ROCM-31245."; | ||
| } | ||
|
|
||
| // Address equality is necessary but not sufficient on its own to notice a | ||
| // regression -- verify that mutations actually propagate, which is the | ||
| // property callers depend on. | ||
| TEST(SingletonIdentity, DebugSingletonStatePropagatesAcrossLinkUnits) | ||
| { | ||
| const TensileLite::StringSet original | ||
| = TensileLite::Debug::Instance().excludedLibFromGetAll(); | ||
|
|
||
| TensileLite::StringSet probe({"GridBasedMatching", "PredictionMatching"}); | ||
| TensileLite::Debug::Instance().setExcludedLibFromGetAll(probe); | ||
|
|
||
| const auto* observed = static_cast<const TensileLite::Debug*>( | ||
| TensileLite::debugInstanceAddress()); | ||
| EXPECT_EQ(observed->excludedLibFromGetAll().size(), 2u) | ||
| << "State written through Debug::Instance() in this link unit is not visible " | ||
| "to libtensilelite-host. See ROCM-31245."; | ||
|
|
||
| TensileLite::StringSet restore(original); | ||
| TensileLite::Debug::Instance().setExcludedLibFromGetAll(restore); | ||
| } | ||
| } // namespace | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will show up in the public headers even though it is a private function used only for testing. Can we put it in a separate header that we compile/export only when we set
TENSILELITE_BUILD_TESTING=ON?