Skip to content
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

[SandboxIR] Add extern templates for GlobalWithNodeAPI #111940

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

fsfod
Copy link
Contributor

@fsfod fsfod commented Oct 11, 2024

These symbols will need to be explicitly exported for SandboxIRTests when LLVM is built as shared library on window with explicitly visibility macros are enabled.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and plugins on window.

These symbols will need to be explicitly exported for SandboxIRTests when LLVM
is built as shared library on window with explicitly visibility macros are enabled.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and plugins on window.
@vgvassilev vgvassilev requested a review from vporpo October 22, 2024 07:35
@vporpo
Copy link
Contributor

vporpo commented Oct 22, 2024

I am trying to understand why these would need to be explicitly exported for SandboxIRTests. The tests don't use the template directly.

There are other similar CRTPs, like the SingleLLVMInstructionImpl in llvm//SandboxIR/Instruction.h, would they also need to be explicitly exported too?

@fsfod
Copy link
Contributor Author

fsfod commented Oct 23, 2024

idk if this helps much but these are the linker errors i get without this change, this is a debug build so a lot of stuff is not inlined:

llvm\lld-link : error : undefined symbol: public: class llvm::sandboxir::GlobalIFunc & __cdecl llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalIFunc, class llvm::GlobalIFunc, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV::operator()(class llvm::GlobalIFunc &) const
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalIFunc & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalIFunc, 1, 0, void, 0, void>, 0, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalIFunc, class llvm::GlobalIFunc, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::GlobalIFunc &>::operator*(void) const)
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalIFunc & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalIFunc, 1, 0, void, 0, void>, 1, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalIFunc, class llvm::GlobalIFunc, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::GlobalIFunc &>::operator*(void) const)
  
llvm\lld-link : error : undefined symbol: public: class llvm::sandboxir::GlobalVariable & __cdecl llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalVariable, class llvm::GlobalVariable, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV::operator()(class llvm::GlobalVariable &) const
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalVariable & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalVariable, 1, 0, void, 0, void>, 0, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalVariable, class llvm::GlobalVariable, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::GlobalVariable &>::operator*(void) const)
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalVariable & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalVariable, 1, 0, void, 0, void>, 1, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalVariable, class llvm::GlobalVariable, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::GlobalVariable &>::operator*(void) const)
  
llvm\lld-link : error : undefined symbol: public: class llvm::sandboxir::GlobalAlias & __cdecl llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalAlias, class llvm::GlobalAlias, class llvm::sandboxir::GlobalValue, class llvm::GlobalValue>::LLVMGVToGV::operator()(class llvm::GlobalAlias &) const
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalAlias & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalAlias, 1, 0, void, 0, void>, 0, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalAlias, class llvm::GlobalAlias, class llvm::sandboxir::GlobalValue, class llvm::GlobalValue>::LLVMGVToGV, class llvm::sandboxir::GlobalAlias &>::operator*(void) const)
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::GlobalAlias & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::GlobalAlias, 1, 0, void, 0, void>, 1, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::GlobalAlias, class llvm::GlobalAlias, class llvm::sandboxir::GlobalValue, class llvm::GlobalValue>::LLVMGVToGV, class llvm::sandboxir::GlobalAlias &>::operator*(void) const)
  
llvm\lld-link : error : undefined symbol: public: class llvm::sandboxir::Function & __cdecl llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::Function, class llvm::Function, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV::operator()(class llvm::Function &) const
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::Function & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Function, 1, 0, void, 0, void>, 0, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::Function, class llvm::Function, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::Function &>::operator*(void) const)
  >>> referenced by llvm\include\llvm\ADT\STLExtras.h:363
  >>>               unittests\SandboxIR\CMakeFiles\SandboxIRTests.dir\SandboxIRTest.cpp.obj:(public: class llvm::sandboxir::Function & __cdecl llvm::mapped_iterator<class llvm::ilist_iterator<struct llvm::ilist_detail::node_options<class llvm::Function, 1, 0, void, 0, void>, 1, 0>, struct llvm::sandboxir::GlobalWithNodeAPI<class llvm::sandboxir::Function, class llvm::Function, class llvm::sandboxir::GlobalObject, class llvm::GlobalObject>::LLVMGVToGV, class llvm::sandboxir::Function &>::operator*(void) const)

There are other similar CRTPs, like the SingleLLVMInstructionImpl in llvm//SandboxIR/Instruction.h, would they also need to be explicitly exported too?

I think those might of got exported from visibility macros that will be added on classes that derived from an instantiation of them like here https://github.com/fsfod/llvm-project/blob/fdf88b914978d57c6c75892a3133d4476b8995d1/llvm/include/llvm/SandboxIR/Instruction.h#L411 I just didn't want add them all in this PR.

Copy link
Contributor

@vporpo vporpo left a comment

Choose a reason for hiding this comment

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

LG, please add a brief comment that this is for LLVM_BUILD_LLVM_DYLIB.

@@ -797,6 +798,24 @@ class GlobalWithNodeAPI : public ParentT {
}
};

extern template LLVM_TEMPLATE_ABI GlobalIFunc &
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment that this is for LLVM_BUILD_LLVM_DYLIB.

@@ -301,6 +302,24 @@ template class GlobalWithNodeAPI<GlobalVariable, llvm::GlobalVariable,
template class GlobalWithNodeAPI<GlobalAlias, llvm::GlobalAlias, GlobalValue,
llvm::GlobalValue>;

template LLVM_EXPORT_TEMPLATE GlobalIFunc &
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@vgvassilev vgvassilev merged commit a905052 into llvm:main Oct 24, 2024
8 checks passed
@frobtech frobtech mentioned this pull request Oct 25, 2024
@bogner
Copy link
Contributor

bogner commented Oct 25, 2024

This doesn't look right - why do we instantiate each of these templates twice? I'm seeing warnings from clang-cl now since there are duplicate instantiations between Constant.h and Constant.cpp.

llvm/lib/SandboxIR/Constant.cpp(309,52): error: duplicate explicit instantiation of 'operator()' ignored as a Microsoft extension [-Werror,-Wmicrosoft-template]
  309 |                   llvm::GlobalObject>::LLVMGVToGV::operator()(llvm::GlobalIFunc
      |                                                    ^
llvm/include\llvm/SandboxIR/Constant.h(804,52): note: previous explicit instantiation is here
  804 |                   llvm::GlobalObject>::LLVMGVToGV::operator()(llvm::GlobalIFunc
      |                                                    ^

bogner added a commit to bogner/llvm-project that referenced this pull request Oct 25, 2024
@fsfod
Copy link
Contributor Author

fsfod commented Oct 25, 2024

I have double checked again for shared library debug builds with clang-cl and the template function symbols are not exported without them being explicitly exported from the source file, explicit template instantiation of containing classes is not enough. I could add some clang warning suppression macros around them though.
My branch with all my stuff still to merge just for clang-cl is here that I checked with.

@bogner
Copy link
Contributor

bogner commented Oct 27, 2024

Do they need to be explicitly exported in both the header and the source file? That doesn't make a lot of sense to me. The instantiations look identical, as the warning implies.

NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
These symbols will need to be explicitly exported for SandboxIRTests
when LLVM is built as shared library on window with explicitly
visibility macros are enabled.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and plugins on
windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants