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

Cuda array interface #617

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

LucasGandel
Copy link
Collaborator

Implement changes required on the RTK side following the implementation of cuda_array_interface in CudaCommon

Requires ITKCudaCommon#42

Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

I would like to be able to compile both the CudaCommon latest release and master's head versions, following RTK's policy for ITK versions. In this case, it might be doable by testing whether GetGPUBufferPointerPtr is defined as is done in eb90fb4 for an ITK macro. Note the regular expression in the log. Do you want to give it a try or should I try to implement this?

src/rtkCudaWarpForwardProjectionImageFilter.cxx Outdated Show resolved Hide resolved
@LucasGandel
Copy link
Collaborator Author

I would like to be able to compile both the CudaCommon latest release and master's head versions, following RTK's policy for ITK versions. In this case, it might be doable by testing whether GetGPUBufferPointerPtr is defined as is done in eb90fb4 for an ITK macro. Note the regular expression in the log. Do you want to give it a try or should I try to implement this?

Should we add a target_compile_definition in ITKCudaCommon to peform such a check (e.g. "CudaDataManagerHasGetGPUBufferPointerPtr")? Or do you prefer to use the CudaCommon_VERSION as suggested in RTKConsortium/ITKCudaCommon#42 ? Or something else ?

@SimonRit
Copy link
Collaborator

SimonRit commented Sep 17, 2024

Should we add a target_compile_definition in ITKCudaCommon to peform such a check (e.g. "CudaDataManagerHasGetGPUBufferPointerPtr")? Or do you prefer to use the CudaCommon_VERSION as suggested in RTKConsortium/ITKCudaCommon#42 ? Or something else ?

No preference, the second one sounds simpler to me. But it is not defined for the latest release so it will just be

#ifdef CUDA_COMMON_MAJOR_VERSION
#else
#endif

I guess

@LucasGandel
Copy link
Collaborator Author

@SimonRit To preserve backward compatibility with CudaCommon, it was also required to add checks in itkCudaImageRTK.wrap, as it uses CudaCommon_SOURCE_DIR which is undefined before CudaCommon 2.0.
CudaCommon_SOURCE_DIR is now also used to check if cudaCommonConfiguration.h exists, which will define CUDACOMMON_VERSION_MAJOR.

The linter is unhappy with the new #ifdef, let me know if you have any other feedback before I fix the style.Thanks

@SimonRit SimonRit modified the milestones: RTK 3.0, RTK 2.7 Dec 4, 2024
Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

This is ready to be merged IMO, thanks for this great work maintaining backward compatibility with the CudaCommon HEAD and the new PR. There are (weird) code style issues:
https://github.com/RTKConsortium/RTK/actions/runs/11798626855/job/32865225878#step:4:6
Can you fix them before I merge please?

@LucasGandel
Copy link
Collaborator Author

This is ready to be merged IMO, thanks for this great work maintaining backward compatibility with the CudaCommon HEAD and the new PR. There are (weird) code style issues: https://github.com/RTKConsortium/RTK/actions/runs/11798626855/job/32865225878#step:4:6 Can you fix them before I merge please?

Thanks! Style issues have been fixed (although they were definitely weird).

@SimonRit SimonRit self-assigned this Dec 6, 2024
Copy link
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the style issue! BTW, ITK is working on an upgrade of clang, see e.g.
InsightSoftwareConsortium/ITK#5012
InsightSoftwareConsortium/ITK#5015
We should plan following this upgrade.

@SimonRit SimonRit merged commit a579b94 into RTKConsortium:master Dec 6, 2024
11 of 59 checks passed
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