Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

[Cython Build] Pass all compile definitions #410

Merged
merged 4 commits into from
Apr 21, 2023

Conversation

Devjiu
Copy link
Contributor

@Devjiu Devjiu commented Apr 17, 2023

This pr passes all compile definitions and include dirs from CMake to cython build.
Also removing dependencies to cuda from library headers.

Resolves: #297

Signed-off-by: Dmitrii Makarenko [email protected]

@Devjiu Devjiu self-assigned this Apr 17, 2023
@Devjiu Devjiu requested a review from ienkovich April 17, 2023 20:39
@Devjiu Devjiu force-pushed the dmitriim/compile_defenitions_to_cython_target branch from 19f5dc0 to 94c124b Compare April 19, 2023 15:31
@Devjiu Devjiu requested a review from kurapov-peter April 19, 2023 15:32
@Devjiu
Copy link
Contributor Author

Devjiu commented Apr 19, 2023

As currently I am changing headers like Bakend.h, Execute.h etc. Adding @kurapov-peter to check, that I am not breaking L0 and current changes fine.

@Devjiu Devjiu force-pushed the dmitriim/compile_defenitions_to_cython_target branch from 94c124b to 4e53440 Compare April 20, 2023 14:40
@Devjiu Devjiu requested a review from gshimansky April 20, 2023 15:12
@Devjiu
Copy link
Contributor Author

Devjiu commented Apr 20, 2023

@gshimansky Please, take a look, it shouldn't fail windows build, but I am not sure how to check.

get_property(CDEFS DIRECTORY PROPERTY COMPILE_DEFINITIONS)
get_property(IDIRS DIRECTORY PROPERTY INCLUDE_DIRECTORIES)
message(STATUS "Defintions to Cython: ${CDEFS}")
message(STATUS "Include dirs to Cython: ${IDIRS}")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I keep the messages or remove them?

@Devjiu Devjiu marked this pull request as ready for review April 20, 2023 16:41
Copy link
Contributor

@ienkovich ienkovich left a comment

Choose a reason for hiding this comment

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

Overall looks good!

@@ -165,7 +171,7 @@ class CUDABackend : public Backend {
llvm::TargetMachine* nvptx_target_machine);

static std::unique_ptr<llvm::TargetMachine> initializeNVPTXBackend(
const CudaMgr_Namespace::NvidiaDeviceArch arch);
const CudaMgr_Namespace::NvidiaDeviceArch varch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks for reminder)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@@ -1,5 +1,10 @@
set(SETUP_PY "${CMAKE_CURRENT_BINARY_DIR}/setup.py")

get_property(CDEFS DIRECTORY PROPERTY COMPILE_DEFINITIONS)
get_property(IDIRS DIRECTORY PROPERTY INCLUDE_DIRECTORIES)
message(STATUS "Defintions to Cython: ${CDEFS}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove debug prints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

Devjiu added 4 commits April 21, 2023 07:03
This pr passes all compile definitions from CMake to cython build.

Resolves: #297

Signed-off-by: Dmitrii Makarenko <[email protected]>
This commit applies style to setup.py.in.

Signed-off-by: Dmitrii Makarenko <[email protected]>
This commits wraps library/include generation into functions.

Signed-off-by: Dmitrii Makarenko <[email protected]>
This commit enables build with cuda and HAVE_CUDA definition. Also
passes include dirs from cmake to setup.

Signed-off-by: Dmitrii Makarenko <[email protected]>
@Devjiu Devjiu force-pushed the dmitriim/compile_defenitions_to_cython_target branch from 4e53440 to bc63efc Compare April 21, 2023 12:03
@Devjiu
Copy link
Contributor Author

Devjiu commented Apr 21, 2023

Note:

Only libraries parameter for all *.pyx remains unchanged. Also can be modified, if libraries can be taken from some specific targets.

@ienkovich ienkovich merged commit 7d12ac2 into main Apr 21, 2023
@ienkovich ienkovich deleted the dmitriim/compile_defenitions_to_cython_target branch April 21, 2023 17:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass all compile definitions to Cython
2 participants