From e26c6d78adc47eb721286f9b0517ac500e03528a Mon Sep 17 00:00:00 2001 From: chengduoZH Date: Wed, 11 Apr 2018 22:42:29 +0800 Subject: [PATCH] code refine --- paddle/fluid/framework/details/CMakeLists.txt | 10 ++-- .../framework/details/broadcast_op_handle.cc | 29 ++++------ .../framework/details/broadcast_op_handle.h | 4 -- .../details/broadcast_op_handle_test.cc | 18 ++++-- .../framework/details/gather_op_handle.cc | 39 ++++++------- .../framework/details/gather_op_handle.h | 4 -- .../details/gather_op_handle_test.cc | 56 ++++--------------- .../fluid/framework/details/op_handle_base.cc | 15 +++++ .../fluid/framework/details/op_handle_base.h | 8 +++ 9 files changed, 83 insertions(+), 100 deletions(-) diff --git a/paddle/fluid/framework/details/CMakeLists.txt b/paddle/fluid/framework/details/CMakeLists.txt index 9c1d1458282..897e41f79f4 100644 --- a/paddle/fluid/framework/details/CMakeLists.txt +++ b/paddle/fluid/framework/details/CMakeLists.txt @@ -1,5 +1,5 @@ cc_library(var_handle SRCS var_handle.cc DEPS place) -cc_library(op_handle_base SRCS op_handle_base.cc DEPS var_handle device_context) +cc_library(op_handle_base SRCS op_handle_base.cc DEPS var_handle device_context lod_tensor) cc_library(scale_loss_grad_op_handle SRCS scale_loss_grad_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory) cc_library(fetch_op_handle SRCS fetch_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory) nv_library(nccl_all_reduce_op_handle SRCS nccl_all_reduce_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory @@ -21,10 +21,10 @@ cc_library(ssa_graph_executor SRCS ssa_graph_executor.cc DEPS ssa_graph framewor cc_library(threaded_ssa_graph_executor SRCS threaded_ssa_graph_executor.cc DEPS fetch_op_handle ssa_graph_executor scope simple_threadpool device_context) -cc_library(broadcast_op_handle SRCS broadcast_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory) -cc_library(gather_op_handle SRCS gather_op_handle.cc DEPS op_handle_base scope lod_tensor ddim memory) +cc_library(broadcast_op_handle SRCS broadcast_op_handle.cc DEPS op_handle_base scope ddim memory) +cc_library(gather_op_handle SRCS gather_op_handle.cc DEPS op_handle_base scope ddim memory) -cc_test(broadcast_op_test SRCS broadcast_op_handle_test.cc DEPS var_handle op_handle_base scope lod_tensor ddim memory +cc_test(broadcast_op_test SRCS broadcast_op_handle_test.cc DEPS var_handle op_handle_base scope ddim memory device_context broadcast_op_handle) -cc_test(gather_op_test SRCS gather_op_handle_test.cc DEPS var_handle op_handle_base scope lod_tensor ddim memory +cc_test(gather_op_test SRCS gather_op_handle_test.cc DEPS var_handle op_handle_base scope ddim memory device_context gather_op_handle) diff --git a/paddle/fluid/framework/details/broadcast_op_handle.cc b/paddle/fluid/framework/details/broadcast_op_handle.cc index 7cd13a50f55..dc8db33ef4e 100644 --- a/paddle/fluid/framework/details/broadcast_op_handle.cc +++ b/paddle/fluid/framework/details/broadcast_op_handle.cc @@ -18,23 +18,16 @@ namespace paddle { namespace framework { namespace details { -static Tensor *GetTensorFromVar(Variable *in_var) { - if (in_var->IsType()) { - return in_var->GetMutable(); - } else if (in_var->IsType()) { - return in_var->GetMutable()->mutable_value(); - } else { - PADDLE_THROW("Var should be LoDTensor or SelectedRows"); - } - return nullptr; -} BroadcastOpHandle::BroadcastOpHandle(const std::vector &local_scopes, const std::vector &places) : local_scopes_(local_scopes), places_(places) {} void BroadcastOpHandle::RunImpl() { - PADDLE_ENFORCE_EQ(this->inputs_.size(), 1); - PADDLE_ENFORCE_EQ(this->outputs_.size(), places_.size()); + PADDLE_ENFORCE_EQ(this->inputs_.size(), 1, + "The number of input should be one."); + PADDLE_ENFORCE_EQ( + this->outputs_.size(), places_.size(), + "The number of output should equal to the number of places."); // Wait input done, this Wait is asynchronous operation auto in_var_handle = static_cast(this->inputs_[0]); @@ -43,7 +36,9 @@ void BroadcastOpHandle::RunImpl() { inputs_[0]->generated_op_->Wait(dev_ctxes_[in_place]); auto in_scope_idx = in_var_handle->scope_idx_; - PADDLE_ENFORCE_LT(in_scope_idx, local_scopes_.size(), ""); + PADDLE_ENFORCE_LT(in_scope_idx, local_scopes_.size(), + "The input(%s) is not in the local_scopes.", + in_var_handle->name_); auto in_var = local_scopes_[in_scope_idx]->FindVar(in_var_handle->name_); Tensor *in_tensor = GetTensorFromVar(in_var); @@ -56,12 +51,8 @@ void BroadcastOpHandle::RunImpl() { "%s is not the the local_scopes ", out_handle->name_); auto *s = local_scopes_[out_scope_idx]; auto out_var = s->FindVar(out_handle->name_); - - PADDLE_ENFORCE_EQ( - out_var->Type(), in_var->Type(), - "The type of input and output is not equal. (%s_%d vs %s_%d)", - out_handle->name_, out_handle->scope_idx_, in_var_handle->name_, - in_var_handle->scope_idx_); + PADDLE_ENFORCE_EQ(out_p.which(), in_place.which(), + "The place of input and output should be the same."); if (in_var->IsType()) { auto &in_sr = in_var->Get(); diff --git a/paddle/fluid/framework/details/broadcast_op_handle.h b/paddle/fluid/framework/details/broadcast_op_handle.h index 74c0a6a0980..b3292422522 100644 --- a/paddle/fluid/framework/details/broadcast_op_handle.h +++ b/paddle/fluid/framework/details/broadcast_op_handle.h @@ -28,10 +28,6 @@ namespace paddle { namespace framework { namespace details { -/* - * Broadcast the input to all scope. - * - */ struct BroadcastOpHandle : public OpHandleBase { const std::vector &local_scopes_; const std::vector &places_; diff --git a/paddle/fluid/framework/details/broadcast_op_handle_test.cc b/paddle/fluid/framework/details/broadcast_op_handle_test.cc index 29cf120c76d..cd069df118f 100644 --- a/paddle/fluid/framework/details/broadcast_op_handle_test.cc +++ b/paddle/fluid/framework/details/broadcast_op_handle_test.cc @@ -17,6 +17,10 @@ #include "paddle/fluid/platform/device_context.h" +namespace paddle { +namespace framework { +namespace details { + namespace f = paddle::framework; namespace p = paddle::platform; @@ -25,7 +29,7 @@ const f::DDim kDims = {20, 20}; class BroadcastTester : public ::testing::Test { public: - void InitCtx(bool use_gpu) { + void InitCtxOnGpu(bool use_gpu) { if (use_gpu) { #ifdef PADDLE_WITH_CUDA int count = p::GetCUDADeviceCount(); @@ -200,23 +204,27 @@ class BroadcastTester : public ::testing::Test { }; TEST_F(BroadcastTester, TestCPUBroadcastTestLodTensor) { - InitCtx(false); + InitCtxOnGpu(false); TestBroadcastLodTensor(); } TEST_F(BroadcastTester, TestCPUBroadcastTestSelectedRows) { - InitCtx(false); + InitCtxOnGpu(false); TestBroadcastSelectedRows(); } #ifdef PADDLE_WITH_CUDA TEST_F(BroadcastTester, TestGPUBroadcastTestLodTensor) { - InitCtx(true); + InitCtxOnGpu(true); TestBroadcastLodTensor(); } TEST_F(BroadcastTester, TestGPUBroadcastTestSelectedRows) { - InitCtx(true); + InitCtxOnGpu(true); TestBroadcastSelectedRows(); } #endif + +} // namespace details +} // namespace framework +} // namespace paddle diff --git a/paddle/fluid/framework/details/gather_op_handle.cc b/paddle/fluid/framework/details/gather_op_handle.cc index 94078683722..3047054d1a0 100644 --- a/paddle/fluid/framework/details/gather_op_handle.cc +++ b/paddle/fluid/framework/details/gather_op_handle.cc @@ -18,23 +18,16 @@ namespace paddle { namespace framework { namespace details { -static Tensor *GetTensorFromVar(Variable *in_var) { - if (in_var->IsType()) { - return in_var->GetMutable(); - } else if (in_var->IsType()) { - return in_var->GetMutable()->mutable_value(); - } else { - PADDLE_THROW("Var should be LoDTensor or SelectedRows"); - } - return nullptr; -} GatherOpHandle::GatherOpHandle(const std::vector &local_scopes, const std::vector &places) : local_scopes_(local_scopes), places_(places) {} void GatherOpHandle::RunImpl() { - PADDLE_ENFORCE_EQ(this->inputs_.size(), places_.size()); - PADDLE_ENFORCE_EQ(this->outputs_.size(), 1); + PADDLE_ENFORCE_EQ( + this->inputs_.size(), places_.size(), + "The number of inputs should be equal to the number of place."); + PADDLE_ENFORCE_EQ(this->outputs_.size(), 1, + "The number of output should be one."); // Wait input done, this Wait is asynchronous operation for (auto *in : inputs_) { @@ -46,6 +39,7 @@ void GatherOpHandle::RunImpl() { auto in_0_handle = static_cast(inputs_[0]); auto pre_in_var = local_scopes_[in_0_handle->scope_idx_]->FindVar(in_0_handle->name_); + auto pre_place = in_0_handle->place_; std::vector out_rows; std::vector in_tensors; @@ -58,7 +52,8 @@ void GatherOpHandle::RunImpl() { in_places.push_back(in_p); PADDLE_ENFORCE_LT(in_handle->scope_idx_, local_scopes_.size(), "%s is not the the local_scopes ", in_handle->name_); - + PADDLE_ENFORCE_EQ(in_p.which(), pre_place.which(), + "The place of input should be the same."); auto *s = local_scopes_[in_handle->scope_idx_]; auto in_var = s->FindVar(in_handle->name_); PADDLE_ENFORCE_EQ(in_var->Type(), pre_in_var->Type(), @@ -69,13 +64,17 @@ void GatherOpHandle::RunImpl() { auto &in_sr = in_var->Get(); auto in_sr_rows = in_sr.rows(); out_rows.insert(out_rows.begin(), in_sr_rows.begin(), in_sr_rows.end()); - PADDLE_ENFORCE_EQ(pre_in.height(), in_sr.height(), ""); - PADDLE_ENFORCE_EQ(pre_in.GetCompleteDims(), in_sr.GetCompleteDims(), ""); + PADDLE_ENFORCE_EQ(pre_in.height(), in_sr.height(), + "The height of inputs is not consistent."); + PADDLE_ENFORCE_EQ(pre_in.GetCompleteDims(), in_sr.GetCompleteDims(), , + "The dims of inputs is not consistent."); } else if (in_var->IsType()) { auto &pre_in = pre_in_var->Get(); auto &in_lodtensor = in_var->Get(); - PADDLE_ENFORCE_EQ(in_lodtensor.lod(), pre_in.lod()); - PADDLE_ENFORCE_EQ(in_lodtensor.dims(), pre_in.dims()); + PADDLE_ENFORCE_EQ(in_lodtensor.lod(), pre_in.lod(), + "The lod of inputs is not consistent."); + PADDLE_ENFORCE_EQ(in_lodtensor.dims(), pre_in.dims(), + "The dims of inputs is not consistent."); } else { PADDLE_THROW("Var should be LoDTensor or SelectedRows."); } @@ -88,7 +87,8 @@ void GatherOpHandle::RunImpl() { auto &out_place = out_handle->place_; auto out_scope_idx = out_handle->scope_idx_; auto out_var = local_scopes_[out_scope_idx]->FindVar(out_handle->name_); - + PADDLE_ENFORCE_EQ(out_place.which(), pre_place.which(), + "The place of input and output should be the same."); if (pre_in_var->IsType()) { auto &pre_in = pre_in_var->Get(); auto out = out_var->GetMutable(); @@ -110,12 +110,13 @@ void GatherOpHandle::RunImpl() { s = e; } } else if (pre_in_var->IsType()) { + // gather LoDTensor ??? } else { PADDLE_THROW("Var should be LoDTensor or SelectedRows."); } } -std::string GatherOpHandle::Name() const { return "broadcast"; } +std::string GatherOpHandle::Name() const { return "gather"; } } // namespace details } // namespace framework } // namespace paddle diff --git a/paddle/fluid/framework/details/gather_op_handle.h b/paddle/fluid/framework/details/gather_op_handle.h index 48e1db227bd..6c0231f642c 100644 --- a/paddle/fluid/framework/details/gather_op_handle.h +++ b/paddle/fluid/framework/details/gather_op_handle.h @@ -28,10 +28,6 @@ namespace paddle { namespace framework { namespace details { -/* - * Broadcast the input to all scope. - * - */ struct GatherOpHandle : public OpHandleBase { const std::vector &local_scopes_; const std::vector &places_; diff --git a/paddle/fluid/framework/details/gather_op_handle_test.cc b/paddle/fluid/framework/details/gather_op_handle_test.cc index a029a2d2666..5d105b37aa8 100644 --- a/paddle/fluid/framework/details/gather_op_handle_test.cc +++ b/paddle/fluid/framework/details/gather_op_handle_test.cc @@ -17,6 +17,9 @@ #include "paddle/fluid/platform/device_context.h" +namespace paddle { +namespace framework { +namespace details { namespace f = paddle::framework; namespace p = paddle::platform; @@ -25,7 +28,7 @@ const f::DDim kDims = {20, 20}; class GatherTester : public ::testing::Test { public: - void InitCtx(bool use_gpu) { + void InitCtxOnGpu(bool use_gpu) { if (use_gpu) { #ifdef PADDLE_WITH_CUDA int count = p::GetCUDADeviceCount(); @@ -103,45 +106,7 @@ class GatherTester : public ::testing::Test { } } - void TestGatherLodTensor() { - // int input_scope_idx = 0; - // InitGatherOp(input_scope_idx); - // - // auto in_var = local_scope_[input_scope_idx]->Var("input"); - // auto in_lod_tensor = in_var->GetMutable(); - // in_lod_tensor->mutable_data(kDims, gpu_list_[input_scope_idx]); - // - // std::vector send_vector(f::product(kDims), input_scope_idx + - // 12); - // for (size_t k = 0; k < send_vector.size(); ++k) { - // send_vector[k] = k; - // } - // f::LoD lod{{0, 10, 20}}; - // paddle::framework::TensorFromVector( - // send_vector, *(ctxs_[input_scope_idx]), in_lod_tensor); - // in_lod_tensor->set_lod(lod); - // - // gather_op_handle_->Run(false); - // - // WaitAll(); - // - // p::CPUPlace cpu_place; - // for (size_t j = 0; j < gpu_list_.size(); ++j) { - // auto out_var = local_scope_[j]->Var("out"); - // auto out_tensor = out_var->Get(); - // PADDLE_ENFORCE_EQ(out_tensor.lod(), lod, "lod is not equal."); - // - // f::Tensor result_tensor; - // f::TensorCopy(out_tensor, cpu_place, *(ctxs_[j]), &result_tensor); - // float* ct = result_tensor.mutable_data(cpu_place); - // - // for (int64_t j = 0; j < f::product(kDims); ++j) { - // ASSERT_NEAR(ct[j], send_vector[j], 1e-5); - // } - // } - // - // GatherOpDestroy(); - } + void TestGatherLodTensor() {} void TestGatherSelectedRows() { int output_scope_idx = 0; @@ -205,23 +170,26 @@ class GatherTester : public ::testing::Test { }; // TEST_F(GatherTester, TestCPUGatherTestLodTensor) { -// InitCtx(false); +// InitCtxOnGpu(false); // TestGatherLodTensor(); //} TEST_F(GatherTester, TestCPUGatherTestSelectedRows) { - InitCtx(false); + InitCtxOnGpu(false); TestGatherSelectedRows(); } #ifdef PADDLE_WITH_CUDA // TEST_F(GatherTester, TestGPUGatherTestLodTensor) { -// InitCtx(true); +// InitCtxOnGpu(true); // TestGatherLodTensor(); //} TEST_F(GatherTester, TestGPUGatherTestSelectedRows) { - InitCtx(true); + InitCtxOnGpu(true); TestGatherSelectedRows(); } #endif +} // namespace details +} // namespace framework +} // namespace paddle diff --git a/paddle/fluid/framework/details/op_handle_base.cc b/paddle/fluid/framework/details/op_handle_base.cc index e4194a7442f..0d7fbdfeab4 100644 --- a/paddle/fluid/framework/details/op_handle_base.cc +++ b/paddle/fluid/framework/details/op_handle_base.cc @@ -17,6 +17,21 @@ namespace paddle { namespace framework { namespace details { + +// GetTensorFromVar is used in broadcast_op handle and gather_op handle, so it +// should be placed in a commonplace. I don't find an appropriate place, so I +// temporarily place it in op_handle_base. +Tensor *GetTensorFromVar(Variable *in_var) { + if (in_var->IsType()) { + return in_var->GetMutable(); + } else if (in_var->IsType()) { + return in_var->GetMutable()->mutable_value(); + } else { + PADDLE_THROW("Var should be LoDTensor or SelectedRows"); + } + return nullptr; +} + std::string OpHandleBase::DebugString() const { std::stringstream ss; ss << "("; diff --git a/paddle/fluid/framework/details/op_handle_base.h b/paddle/fluid/framework/details/op_handle_base.h index d7a541ac4bb..fedff07772e 100644 --- a/paddle/fluid/framework/details/op_handle_base.h +++ b/paddle/fluid/framework/details/op_handle_base.h @@ -17,6 +17,9 @@ #include #include "paddle/fluid/framework/details/var_handle.h" +#include "paddle/fluid/framework/lod_tensor.h" +#include "paddle/fluid/framework/selected_rows.h" +#include "paddle/fluid/framework/variable.h" #include "paddle/fluid/platform/device_context.h" #include "paddle/fluid/platform/macros.h" @@ -24,6 +27,11 @@ namespace paddle { namespace framework { namespace details { +// GetTensorFromVar is used in broadcast_op handle and gather_op handle, so it +// should be placed in a commonplace. I don't find an appropriate place, so I +// temporarily place it in op_handle. +Tensor *GetTensorFromVar(Variable *in_var); + class OpHandleBase { private: DISABLE_COPY_AND_ASSIGN(OpHandleBase); -- GitLab