-
Notifications
You must be signed in to change notification settings - Fork 28
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
Ct int solver #189
Ct int solver #189
Conversation
retest this please. |
retest this, please. |
retest this, please |
fixed weight initialization in 2d6f92c |
Test pass, only failure is the distributed G4 test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the semantics of CudaStream and MagmaQueue this looks good. I will also do some testing with it on non summit systems while I wait for your responses and changes.
|
||
namespace dca { | ||
namespace linalg { | ||
namespace util { | ||
// dca::linalg::util:: | ||
|
||
class MagmaQueue { | ||
class MagmaQueue : public linalg::util::CudaStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a MagmaQueue has a CudaStream I don't think it is a CudaStream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could argue if a MagmaQueue is a CudaStream with extra resources. But according to the Liskov Substitution Principle, inheritance is appropriate: everywhere you can use a CudaStream you can also use a MagmaQueue. I set the destructor of the base class to virtual for safety reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its pretty clear a CudaStream is a resource a MagmaQueue owns or has a reference to. When Magma is build on top of AMD's libraries it clearly won't have a CudaStream or be a cudastream. I don't like to see inheritance used as a way to get simplified access to class members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AMD HIP tries to follow CUDA. I think there is still "HIP stream" that works like a cuda stream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you just need to change the implementation of CudaStream (and possibly rename it), this is completely orthogonal from the decision of using composition vs inheritance.
The inheritance has no access to the private members, but it exposes the interface, which I want, and has a well defined downcast that I also need. Of course it could be implemented as composition, but then I need to forward the methods, and use a getter to the stream in the rest of the code. While we don't need runtime polymorphism, I am pretty sure this is a well defined inheritance, and the added 64 bits of storage for the (unused) vtable pointer is a non issue.
include/dca/phys/dca_step/cluster_solver/ctint/accumulator/ctint_accumulator.hpp
Show resolved
Hide resolved
include/dca/phys/dca_step/cluster_solver/shared_tools/accumulation/tp/tp_accumulator.hpp
Outdated
Show resolved
Hide resolved
I still don't understand the crusade against inheritance to represent a simple instance of subtyping, especially given our PR backlog, but I fixed and merged @PDoakORNL PR removing inheritance. |
With this PR, the CT-INT solver will finally be usable.
This PR includes: