Skip to content

Conversation

@paperchalice
Copy link
Contributor

@paperchalice paperchalice commented Nov 13, 2024

The comment about it is 8 years ago, we have newer VS and C++17. Try to move analysis keys to AnalysisInfoMixin.
If this change is feasible on Windows, I will remove analysis keys in analysis passes later.

@llvmbot
Copy link
Member

llvmbot commented Nov 13, 2024

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

The comment about it is 8 years ago, we have newer VS and C++17. Try to move analysis keys to AnalysisInfoMixin.


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

1 Files Affected:

  • (modified) llvm/include/llvm/IR/PassManager.h (+4-10)
diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h
index d269221fac0701..69643d6cab8fc2 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -95,21 +95,15 @@ struct AnalysisInfoMixin : PassInfoMixin<DerivedT> {
   /// This ID is a pointer type that is guaranteed to be 8-byte aligned and thus
   /// suitable for use in sets, maps, and other data structures that use the low
   /// bits of pointers.
-  ///
-  /// Note that this requires the derived type provide a static \c AnalysisKey
-  /// member called \c Key.
-  ///
-  /// FIXME: The only reason the mixin type itself can't declare the Key value
-  /// is that some compilers cannot correctly unique a templated static variable
-  /// so it has the same addresses in each instantiation. The only currently
-  /// known platform with this limitation is Windows DLL builds, specifically
-  /// building each part of LLVM as a DLL. If we ever remove that build
-  /// configuration, this mixin can provide the static key as well.
   static AnalysisKey *ID() {
     static_assert(std::is_base_of<AnalysisInfoMixin, DerivedT>::value,
                   "Must pass the derived type as the template argument!");
     return &DerivedT::Key;
   }
+
+private:
+  /// Opaque, unique ID for this analysis type.
+  static constexpr AnalysisKey Key = {};
 };
 
 namespace detail {

@paperchalice
Copy link
Contributor Author

CC @fsfod because you are working on dynamic library support on Windows.

@fsfod
Copy link
Contributor

fsfod commented Nov 13, 2024

I remember I had to work around with a clang-cl bug that was fixed in v19 to get these to export correctly.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A local build in DLL mode would definitely be good to run against the minimum supported VS (which I believe is 2019).

If that compiler is able to deal with this properly, addressing this makes sense. I do wonder if this is going to break any out-of-tree analysis passes though due to the redeclaration.

@paperchalice
Copy link
Contributor Author

Should be fine if ID returns &AnalysisInfoMixin::Key. Looks like it's not time yet because we need a new enough clang-cl.

@aeubanks
Copy link
Contributor

Should be fine if ID returns &AnalysisInfoMixin::Key. Looks like it's not time yet because we need a new enough clang-cl.

Can you post more details on which version of clang-cl is broken?

@fsfod
Copy link
Contributor

fsfod commented Nov 14, 2024

I should said the issue with the my new DLL build setup not the old existing system that filters exports with a python script, The current changes from this PR doesn't make it any worse or regress the new DLL build setup at least.

The original issue i had was this field wasn't getting imported from a extern explicit template instantiation declaration annotated with dllimport.
The missing symbols were:

llvm\lld-link : error : undefined symbol: private: static struct llvm::AnalysisKey llvm::InnerAnalysisManagerProxy<class llvm::AnalysisManager<class llvm::Function>, class llvm::Module>::Key
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(public: __cdecl `anonymous namespace'::CGSCCPassManagerTest::CGSCCPassManagerTest(void))
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(public: virtual class std::unique_ptr<struct llvm::detail::AnalysisResultConcept<class llvm::Module, class llvm::AnalysisManager<class llvm::Module>::Invalidator>, struct std::default_delete<struct llvm::detail::AnalysisResultConcept<class llvm::Module, class llvm::AnalysisManager<class llvm::Module>::Invalidator>>> __cdecl llvm::detail::AnalysisPassModel<class llvm::Module, class llvm::LazyCallGraphAnalysis, class llvm::AnalysisManager<class llvm::Module>::Invalidator>::run(class llvm::Module &, class llvm::AnalysisManager<class llvm::Module> &))
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(private: virtual class llvm::PreservedAnalyses __cdecl std::_Func_impl_no_alloc<class `private: virtual void __cdecl `anonymous namespace'::CGSCCPassManagerTest_TestModulePassInvalidatesSCCAnalysis_Test::TestBody(void)'::`1'::<lambda_18>, class llvm::PreservedAnalyses, class PreservedAnalyses::Module &, class PreservedAnalyses::AnalysisManager<class llvm::Module> &>::_Do_call(class llvm::Module &, class llvm::AnalysisManager<class llvm::Module> &))
  >>> referenced 4 more times
  
lld-link : error : undefined symbol: private: static struct llvm::AnalysisKey llvm::InnerAnalysisManagerProxy<class llvm::AnalysisManager<class llvm::LazyCallGraph::SCC, class llvm::LazyCallGraph &>, class llvm::Module>::Key
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(public: __cdecl `anonymous namespace'::CGSCCPassManagerTest::CGSCCPassManagerTest(void))
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(private: virtual class llvm::PreservedAnalyses __cdecl std::_Func_impl_no_alloc<class `private: virtual void __cdecl `anonymous namespace'::CGSCCPassManagerTest_TestModulePassInvalidatesSCCAnalysis_Test::TestBody(void)'::`1'::<lambda_18>, class llvm::PreservedAnalyses, class PreservedAnalyses::Module &, class PreservedAnalyses::AnalysisManager<class llvm::Module> &>::_Do_call(class llvm::Module &, class llvm::AnalysisManager<class llvm::Module> &))
  >>> referenced by unittests\Analysis\CMakeFiles\AnalysisTests.dir\CGSCCPassManagerTest.cpp.obj:(private: virtual class llvm::PreservedAnalyses __cdecl std::_Func_impl_no_alloc<class `private: virtual void __cdecl `anonymous namespace'::CGSCCPassManagerTest_TestModulePassCanPreserveSCCAnalysis_Test::TestBody(void)'::`1'::<lambda_20>, class llvm::PreservedAnalyses, class PreservedAnalyses::Module &,

I double checked building with clang-cl 19 and this wasn't fixed so idk if I got confused with some other dllexport bug.

@paperchalice
Copy link
Contributor Author

Unfortunately the address is still not unique in pass plugin and I can't reproduce it locally.

@paperchalice paperchalice deleted the analysis-key branch November 17, 2024 04:52
@paperchalice
Copy link
Contributor Author

There is still something like:

template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
AnalysisKey
InnerAnalysisManagerProxy<AnalysisManagerT, IRUnitT, ExtraArgTs...>::Key;

which would be an issue in future...

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants