-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add tensor.h #2611
Add tensor.h #2611
Conversation
paddle/framework/tensor.h
Outdated
public: | ||
explicit Tensor(DDim dims) : dims_(dims), place_(get_place()) {} | ||
explicit Tensor(DDim dims, Place place) : dims_(dims), place_(place) {} | ||
|
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.
Missing GetDim()
and GetPlace()
?
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 am not sure when would they be necessary, but we can add them in the future once we know we need them.
paddle/framework/tensor.h
Outdated
explicit Tensor(DDim dims, Place place) : dims_(dims), place_(place) {} | ||
|
||
template <typename T> | ||
const T* data() const { |
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.
Should function name be Data()
? https://google.github.io/styleguide/cppguide.html#Function_Names
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.
Cite from the link you pasted:
Accessors and mutators (get and set functions) may be named like variables. These often correspond to actual member variables, but this is not required. For example, int count() and void set_count(int count).
Here we use data
and mutable_data
since they are accessors of Tensor.
paddle/framework/tensor.h
Outdated
|
||
std::unique_ptr<Placeholder> holder_; // holds the memory block if allocated. | ||
DDim dims_; // could be smallers than the holder_->Size(). | ||
paddle::platform::Place place_; |
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 wonder why the place_
is both located in Tensor
and PlaceholderImpl
at first. From the line 39, we can see that the Place
in PlaceholderImpl
is always equal to it in Tensor
.
But it seems reasonable the user can use mutable_data(DDim dims, Place place)
to get a pointer whose Place
is different from the existed holder_
.
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 are right. The current syntax of Tensor is that we can allocate memory in either of the following two ways:
Tensor t(dim, place);
int* p = t.mutable_data<int>();
or
Tensor t;
int* p = t.mutable_data(dim, place);
Should we remove one of them? @qingqing01
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.
Done. Removed the first choice.
paddle/framework/tensor.h
Outdated
template <typename T> | ||
struct PlaceholderImpl : public Placeholder { | ||
PlaceholderImpl(Place pl, size_t size) | ||
: ptr_(memory::Alloc(pl, size), paddle::memory::Deleter(pl)), |
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 don't need to define Deleter
, because we can use lamda function in shared_ptr's reset function.
holder_.reset(new PlaceholderImpl(place_, dims.product() * sizeof(T)), [&](){
....
memory::Free(place_, ptr_.get());
});
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 knew that we can use lambda or anything that casts to std::function<void(T*)>
, but I think we need a class here to save the place and size.
paddle/framework/tensor.h
Outdated
@@ -0,0 +1,76 @@ | |||
/* | |||
Copyright (c) 2016 PaddlePaddle Authors. All Rights Reserve. |
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.
The copyright message has a different style with former,what format should we use in the newly added code?
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.
Done. Switched back to the former one.
Here is my implementation draft based on current design: #2645. Please review it. |
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.
LGTM
This is a work in progress. It's primary purpose is to verify my ideas presented in the design of tensor at #2579 (review).