Skip to content

[LLVM] Register factory function for CodeGenCPU#11852

Merged
leandron merged 3 commits intoapache:mainfrom
kparzysz-quic:codegen-cpu-factory
Jun 23, 2022
Merged

[LLVM] Register factory function for CodeGenCPU#11852
leandron merged 3 commits intoapache:mainfrom
kparzysz-quic:codegen-cpu-factory

Conversation

@kparzysz-quic
Copy link
Contributor

Any target that has its own subclass of CodeGenLLVM must register a factory function that constructs an object of that class. This factory will then be looked up and used in CodeGenLLVM::Create, which is the generic interface to create an LLVM code generator.

However, there is no factory for CodeGenCPU, and so the creation of a CodeGenCPU object is done inside of CodeGenLLVM::Create. To make this happen, codegen_llvm.cc includes codegen_cpu.h, which makes the base class implementation depend on the derived class. This backwards dependency can be resolved by registering a factory for CodeGenCPU.

Any target that has its own subclass of `CodeGenLLVM` must register
a factory function that constructs an object of that class. This
factory will then be looked up and used in `CodeGenLLVM::Create`,
which is the generic interface to create an LLVM code generator.

However, there is no factory for `CodeGenCPU`, and so the creation
of a `CodeGenCPU` object is done inside of `CodeGenLLVM::Create`.
To make this happen, codegen_llvm.cc includes codegen_cpu.h, which
makes the base class implementation depend on the derived class.
This backwards dependency can be resolved by registering a factory
for `CodeGenCPU`.
@leandron
Copy link
Contributor

Thanks for the PR @kparzysz-quic. Is there any chance we can have a test case for this fix?

@kparzysz-quic
Copy link
Contributor Author

I'm not sure what such a testcase should do. The main motivation for this change was to avoid the "reverse dependency" between .cc and .h files. The aren't any tests that I know of that are specific to the factory functions, but if you have any suggestions, let me know.

@leandron
Copy link
Contributor

I'm not sure what such a testcase should do. The main motivation for this change was to avoid the "reverse dependency" between .cc and .h files. The aren't any tests that I know of that are specific to the factory functions, but if you have any suggestions, let me know.

Following the logic being introduced in this PR, I suppose tests could assert that two calls for the factory function will generate two different instances; another test could assert that an invalid target would return nullptr.

Does that make sense?

@kparzysz-quic kparzysz-quic requested a review from leandron June 23, 2022 18:12
@kparzysz-quic
Copy link
Contributor Author

kparzysz-quic commented Jun 23, 2022

Edit: the comment below is wrong.

I'll paste the commit comment about why I deleted the cpp test:

The WASM docker has an installation of LLVM without versioned symbols, but the default compiler uses symbol versions. The added test had a reference to _ZN4llvm24DisableABIBreakingChecksE@@LLVM_11, but the LLVM library had the unversioned symbol:

File: /usr/lib/llvm-11/lib/libLLVMSupport.a(ABIBreak.cpp.o)
     1: 0000000000000000     4 OBJECT  GLOBAL DEFAULT    2 _ZN4llvm24DisableABIBreakingChecksE

Edit: the error message was

/usr/bin/ld: CMakeFiles/cpptest.dir/tests/cpp/llvm_codegen_test.cc.o: undefined reference to symbol '_ZN4llvm24DisableABIBreakingChecksE@@LLVM_11'
//usr/lib/llvm-11/lib/libLLVM-11.so.1: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

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

Thanks @kparzysz-quic! Merging it now.

@leandron leandron merged commit b4c0bf7 into apache:main Jun 23, 2022
@kparzysz-quic kparzysz-quic deleted the codegen-cpu-factory branch June 24, 2022 13:09
blackkker pushed a commit to blackkker/tvm that referenced this pull request Jul 7, 2022
* [LLVM] Register factory function for CodeGenCPU

Any target that has its own subclass of `CodeGenLLVM` must register
a factory function that constructs an object of that class. This
factory will then be looked up and used in `CodeGenLLVM::Create`,
which is the generic interface to create an LLVM code generator.

However, there is no factory for `CodeGenCPU`, and so the creation
of a `CodeGenCPU` object is done inside of `CodeGenLLVM::Create`.
To make this happen, codegen_llvm.cc includes codegen_cpu.h, which
makes the base class implementation depend on the derived class.
This backwards dependency can be resolved by registering a factory
for `CodeGenCPU`.

* Add missing factory functions for other targets

* Add cpp tests for codegen factories
mikeseven pushed a commit to mikeseven/tvm that referenced this pull request Sep 27, 2023
* [LLVM] Register factory function for CodeGenCPU

Any target that has its own subclass of `CodeGenLLVM` must register
a factory function that constructs an object of that class. This
factory will then be looked up and used in `CodeGenLLVM::Create`,
which is the generic interface to create an LLVM code generator.

However, there is no factory for `CodeGenCPU`, and so the creation
of a `CodeGenCPU` object is done inside of `CodeGenLLVM::Create`.
To make this happen, codegen_llvm.cc includes codegen_cpu.h, which
makes the base class implementation depend on the derived class.
This backwards dependency can be resolved by registering a factory
for `CodeGenCPU`.

* Add missing factory functions for other targets

* Add cpp tests for codegen factories
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.

2 participants