Enable tensor test #2891
Enable tensor test #2891
Conversation
| paddle::platform::Place place_; // record the place of ptr_. | ||
| size_t size_; // size of the memory block. | ||
| std::unique_ptr<T, Deleter<PlaceType>> ptr_; | ||
| platform::Place place_; // record the place of ptr_. |
There was a problem hiding this comment.
Since PlaceholderImpl has a template parameter PlaceType, why we need platform::Place here?
There was a problem hiding this comment.
GPUPlace(0), GPUPlace(1), they are different.
There was a problem hiding this comment.
GPUPlace and CPUPlace are two different class types. But GPUPlace(0) and GPUPlace(1) are the same class type.
There was a problem hiding this comment.
Correct. So if we remove platform::Place place_ and only retain template parameter PlaceType, PlaceholderImpl is not able to distinguish GPUPlace(0) and GPUPlace(1).
There was a problem hiding this comment.
With platform::Place here we can easily use == to check two place in mutable_data() and data().
paddle/framework/CMakeLists.txt
Outdated
| @@ -2,7 +2,7 @@ | |||
| cc_library(ddim SRCS ddim.cc) | |||
| cc_test(ddim_test SRCS ddim_test.cc DEPS ddim) | |||
| nv_test(dim_test SRCS dim_test.cu DEPS ddim) | |||
| cc_test(tensor_test SRCS tensor_test.cc DEPS ddim) | |||
| cc_test(tensor_test SRCS tensor_test.cc DEPS ddim place paddle_memory) | |||
There was a problem hiding this comment.
Please update develop, because this lines are changed.
| struct PlaceholderImpl : public Placeholder { | ||
| private: | ||
| template <typename PType> |
There was a problem hiding this comment.
Is that PType useful? Maybe in Deleter, we can also use PlaceType
| @@ -166,7 +179,7 @@ class Tensor { | |||
| DDim dims_; | |||
| size_t numel_; // cache of `product(dims_)` | |||
| size_t offset_; // marks the begin of tensor data area. | |||
| }; | |||
| }; // namespace framework | |||
There was a problem hiding this comment.
above line seems to be a typo.
| PADDLE_ENFORCE(numel_ > 0, | ||
| "Tensor::numel_ must be larger than zero to call " | ||
| "Tensor::mutable_data. Call Tensor::set_dim first."); | ||
| if (holder_ == nullptr || | ||
| !(holder_->place() == | ||
| place) /* some versions of boost::variant don't have operator!= */ | ||
| || holder_->size() < numel_ * sizeof(T) + offset_) { | ||
| holder_.reset(new PlaceholderImpl<T>(place, numel_ * sizeof(T))); | ||
| #ifdef __CUDACC__ |
There was a problem hiding this comment.
#ifdef PADDLE_ONLY_GPU, use that macro.
| boost::get<platform::CPUPlace>(place), numel_ * sizeof(T))); | ||
| break; | ||
| } | ||
| #else |
There was a problem hiding this comment.
if (is_cpu_place(place)) {
holder_.reset(...)
} else if (is_gpu_place(place)) {
#ifdef PADDLE_ONLY_CPU
PADDLE_THROW("GPU is not enabled");
#else
holder_.reset(...)
#endif
} else {
PADDLE_THROW("not supported device.")
}
… dev_enable_tensor_test
paddle::memoryis merged, so I enable tenor's tests.Because of some interface changes, tensor's current code might be a little rough. I will keep polishing it.