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

linalg sum method #3278

Closed

Conversation

OXPHOS
Copy link
Member

@OXPHOS OXPHOS commented Jun 10, 2016

No description provided.

#ifdef HAVE_VIENNACL
return viennacl::linalg::sum(vec.gpuarray->GPUvec());
#else
SG_SERROR("User did not register GPU backend. \n");
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it if we would have to put this error message in every implementation.

Minor: No space after period.

Copy link
Member

Choose a reason for hiding this comment

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

why do we generate at all GPUBackend class if there's no gpu backend? i'm sorry but i'm very very confused what's happening here...

you should implement a GPUBackend using for example viannacl IF it's available. if not you just don't create one. like many other models, that if LAPACK was not present, the implementation was not built.

@lambday
Copy link
Member

lambday commented Jun 12, 2016

@OXPHOS hey!

Can you quickly change the implementation of vector class based on what we had discussed? It is better to be done in the initial phase before we add too many methods (easy to change now than later). So the idea is that there will only be one class for vector, something like

class GPUVectorImpl; // this is the GPUArray thing that you have

template <typename T>
class Vector
{
public:
    Vector(SGVector<T> const &);
    Vector& operator=(SGVector<T> const &);
    operator SGVector<T>() const;
    bool onGPU() const;
    T* data();
    T const * data() const;
    index_t size() const;
private:
    bool m_onGPU;   // this variable would be assessed in every linalg call,
                    // makes sense to put this first
    index_t m_len;  // same logic
    T* m_data;      // non-owning ptr, referring to the SGVector.vector
    unique_ptr<GPUVectorImpl> m_gpu_impl;
    void transferToGPU();
    void transferToCPU();
};

So, every time a call to transferToGPU() is made (by SGLinalg class), the flag onGPU is set to be true. The main thing to keep in mind here: we need to make sure that the CPU and GPU data are in sync.

  • We need to ensure that we invalidate the GPU data if the CPU data is updated. Say, someone transfers the data to GPU, then (s)he assigns another SGVector to it (m_data points to a different memory now). Then the data on GPU is no more valid. Subsequent calls to linalg methods should use the CPU data then.
// vec1 and vec2 are SGVectors
auto v = sg_linalg->transferToGPU(vec1);
// do something with v
v = vec2;
// v.onGPU() == ?, should be false

To achieve this, all we have to do is the set onGPU is false every time the non-const method data() is invoked. Then as long as we implement the assignment operator with the non const data() method we're good.

  • Similarly, if the data is on GPU and we perform some in-place operations, then the CPU data is obsolete. We need to make sure we call the transferToCPU() method in the operator that returns a SGVector then (after all the linalg operations we need to return a SGVector finally anyway).
  • The linalg methods that take a Vector<T> const & as parameters (e.g. dot, sum, etc), will make calls to the const version of data so it's all good. Methods that performs in-place operations will simply pass a non-const reference. Notice that we won't pass the vector as a ptr anymore. It can live on stack (just like SGVector).

Also, think a bit about thread safety while updating the onGPU variable. Please let me know if you have any questions about this. This should be much cleaner than what we have now - hate the name "BaseVector" for a class name :D

@karlnapf @vigsterkr @lisitsyn please share your thoughts and suggestions.

@lambday
Copy link
Member

lambday commented Jun 12, 2016

Maybe it is a good idea to keep the refcount as well?

@OXPHOS OXPHOS mentioned this pull request Jun 13, 2016
@OXPHOS OXPHOS closed this Jun 22, 2016
@OXPHOS OXPHOS deleted the linalg_sum branch June 29, 2016 11:03
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

4 participants