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

Gpu #411

Draft
wants to merge 69 commits into
base: v3
Choose a base branch
from
Draft

Gpu #411

wants to merge 69 commits into from

Conversation

kbidzhiev
Copy link

I've added a comment in options

Copy link
Contributor

@emstoudenmire emstoudenmire left a comment

Choose a reason for hiding this comment

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

Nice to see the changes needed to include CUDA support aren't very big, and it can be included as another "platform" type, mainly changing lapack_wrap.cc. One thing we should discuss is the experience for users who are not going to opt into using CUDA. For example, will they be able to compile the code without having the Thrust library as a dependency? Also if the platform is set to enable the CUDA backend, does all the code now run on GPU or does it require that to still be turned on at runtime (I would prefer the second option).

@@ -18,6 +18,7 @@

#include "itensor/util/print.h"
#include "itensor/util/stdx.h"
#include <thrust/device_vector.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this include make it remain possible for people to compile ITensor if they aren't going to use CUDA or thrust? Also I'm not familiar with thrust: is it related to GPU or something else?

itensor/types.h Outdated
@@ -28,18 +28,30 @@
namespace itensor {

using Real = double;
using Cplx = std::complex<double>;
using Complex = std::complex<double>;
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment block should be removed if not needed, thanks.

@emstoudenmire
Copy link
Contributor

Thanks, this looks like an interesting PR but some aspects of it aren't obvious to me. Could you please describe the changes at a high level? Also I made some review comments above and in the code changes page.

@kbidzhiev kbidzhiev marked this pull request as draft May 18, 2022 13:24
@darcangelomauro
Copy link

Hi @emstoudenmire, thanks for taking the time to look at the code. The PR was sent by mistake, we intended to do some more work on the local fork before showing you guys. We will mark it as a draft PR so that you know you shouldn't spend too much time looking at it.

@emstoudenmire
Copy link
Contributor

Thanks - sounds good. We can consider having additional dependencies such as Thrust but by default these should be turned off or not required if the CUDA backend is not being enabled. Also would be good to discuss whether the CUDA backend can be turned on as a runtime option or not in our further discussions. Looking forward -

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.

None yet

3 participants