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

Few fixes for CUDA #169

Merged
merged 5 commits into from
Apr 21, 2022
Merged

Few fixes for CUDA #169

merged 5 commits into from
Apr 21, 2022

Conversation

krzysg
Copy link
Member

@krzysg krzysg commented Apr 19, 2022

Hello,

Here are few fixes:

  1. Removed 'extra_smooth' parameter as agreed with Joel some time ago (+/- 1 year ;-) ).
  2. Added missing explicit instantiations for types needed by APRTest.cpp
  3. Switching off two big pipeline test - I will work on them in the near future and enable when whole pipeline is working again...
  4. Because pipeline is not working I turned it off in APRConverter.cpp (so CPU is always used regardless off APR_USE_CUDA). It allows to have working tests and progress on CUDA impl.

Those changes allows to compile LibAPR and run tests for CUDA which is minimum for later development and would be nice to have it on develop.

@krzysg krzysg requested a review from joeljonsson April 20, 2022 10:13
@joeljonsson
Copy link
Collaborator

Thanks @krzysg !

The windows tests should pass after updating vcpkg (e.g. after #163 or #168 are merged in)

src/algorithm/APRConverter.hpp Outdated Show resolved Hide resolved
@joeljonsson
Copy link
Collaborator

I'm thinking more along the lines of

public:
    void get_apr(...)         // CPU version

#ifdef APR_USE_CUDA
    void get_apr_cuda(...)    // CUDA version
#endif

such that both versions are callable by the user. This just requires the initial checks from get_apr to be moved into the cpu/gpu methods. Alternatively, if you want to keep the behavior of get_apr the way it is, you can make both get_apr_cpu and get_apr_gpu public in the same way, and then just alias get_apr to one of them based on cuda availability.

@krzysg
Copy link
Member Author

krzysg commented Apr 21, 2022

I think that get_apr should stay as it was in terms of behavior (so used pipeline should depend on APR_USE_CUDA value).
I have change get_apr_cpu(cuda) to public so they can be used explicitly to force using specific pipeline.

@joeljonsson
Copy link
Collaborator

I think that get_apr should stay as it was in terms of behavior (so used pipeline should depend on APR_USE_CUDA value). I have change get_apr_cpu(cuda) to public so they can be used explicitly to force using specific pipeline.

Excellent

@krzysg krzysg merged commit 4244642 into develop Apr 21, 2022
@krzysg krzysg deleted the developKrzysztof branch April 21, 2022 11:48
@krzysg krzysg mentioned this pull request Apr 22, 2022
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