From 281644cd0734d99151b08f8e221c2fd58a326249 Mon Sep 17 00:00:00 2001 From: Chen Weihang Date: Thu, 13 Jan 2022 11:15:49 +0800 Subject: [PATCH] Fix mkldnn invalid infershape impl (#38837) * fix mkldnn invalid infershape * add unittest for mkldnn in new executor * add import os --- .../fluid/eager/legacy/infer_shape_context.h | 19 ++++++++++++++----- .../fluid/eager/legacy/prepared_operator.cc | 2 +- .../new_executor/new_executor_defs.cc | 11 +++++++++++ .../new_executor/new_executor_defs.h | 2 ++ paddle/fluid/framework/op_desc.cc | 4 ++++ paddle/fluid/framework/operator.cc | 15 ++++++++++++--- paddle/fluid/framework/operator.h | 7 ++----- paddle/fluid/framework/shape_inference.h | 2 ++ paddle/fluid/imperative/infer_shape_context.h | 19 +++++++++++++------ paddle/fluid/imperative/prepared_operator.cc | 8 ++++---- paddle/fluid/operators/batch_norm_op.cc | 6 +++--- paddle/fluid/operators/conv_op.cc | 2 +- paddle/fluid/operators/conv_transpose_op.cc | 4 ++-- paddle/fluid/operators/inplace_abn_op.cc | 8 ++++---- paddle/fluid/operators/pool_op.cc | 2 +- .../unittests/mkldnn/test_conv2d_mkldnn_op.py | 10 ++++++++++ 16 files changed, 86 insertions(+), 35 deletions(-) diff --git a/paddle/fluid/eager/legacy/infer_shape_context.h b/paddle/fluid/eager/legacy/infer_shape_context.h index 7a05f6a9b3581..a1032fd404f85 100644 --- a/paddle/fluid/eager/legacy/infer_shape_context.h +++ b/paddle/fluid/eager/legacy/infer_shape_context.h @@ -31,15 +31,18 @@ class EagerInferShapeContext : public paddle::framework::InferShapeContext { using DDim = paddle::framework::DDim; public: - EagerInferShapeContext(const NameTensorMap* in, const NameTensorMap* out, - const paddle::framework::AttributeMap* attr, - const paddle::framework::AttributeMap* default_attr, - const std::string op_type) + EagerInferShapeContext( + const NameTensorMap* in, const NameTensorMap* out, + const paddle::framework::AttributeMap* attr, + const paddle::framework::AttributeMap* default_attr, + const std::string op_type, + const paddle::framework::OpKernelType* op_kernel_type = nullptr) : tensor_in_(in), tensor_out_(out), attrs_(attr), default_attrs_(default_attr), - op_type_(op_type) {} + op_type_(op_type), + op_kernel_type_(op_kernel_type) {} bool HasInput(const std::string& name) const override { // has only one input @@ -214,6 +217,11 @@ class EagerInferShapeContext : public paddle::framework::InferShapeContext { bool IsRuntime() const override { return true; } + bool IsRunMKLDNNKernel() const override { + return (op_kernel_type_ && (op_kernel_type_->data_layout_ == + paddle::framework::DataLayout::kMKLDNN)); + } + // TODO(paddle-dev): Can this be template? std::vector GetInputVarPtrs( const std::string& name) const override { @@ -400,6 +408,7 @@ class EagerInferShapeContext : public paddle::framework::InferShapeContext { const paddle::framework::AttributeMap* attrs_; const paddle::framework::AttributeMap* default_attrs_; const std::string op_type_; + const paddle::framework::OpKernelType* op_kernel_type_; }; } // namespace legacy diff --git a/paddle/fluid/eager/legacy/prepared_operator.cc b/paddle/fluid/eager/legacy/prepared_operator.cc index 4e892b14a9c9c..fbf2d678740ab 100644 --- a/paddle/fluid/eager/legacy/prepared_operator.cc +++ b/paddle/fluid/eager/legacy/prepared_operator.cc @@ -173,7 +173,7 @@ static void PreparedOpRunImpl( paddle::framework::Scope scope; EagerInferShapeContext infer_shape_ctx(&ins, &outs, &attrs, &default_attrs, - op.Type()); + op.Type(), &kernel_type); op.Info().infer_shape_(&infer_shape_ctx); func(EagerExecutionContext(op, scope, *dev_ctx, ctx, ins, outs, attrs, diff --git a/paddle/fluid/framework/new_executor/new_executor_defs.cc b/paddle/fluid/framework/new_executor/new_executor_defs.cc index 4b9404fd178fd..654746794da4e 100644 --- a/paddle/fluid/framework/new_executor/new_executor_defs.cc +++ b/paddle/fluid/framework/new_executor/new_executor_defs.cc @@ -307,6 +307,17 @@ void InterpretercoreInferShapeContext::SetLoDLevel(const std::string& out, bool InterpretercoreInferShapeContext::IsRuntime() const { return true; } +bool InterpretercoreInferShapeContext::IsRunMKLDNNKernel() const { + try { + auto& op_with_kernel = dynamic_cast(op_); + return ((op_with_kernel.kernel_type()) && + (op_with_kernel.kernel_type()->data_layout_ == + framework::DataLayout::kMKLDNN)); + } catch (std::bad_cast exp) { + return false; + } +} + // TODO(paddle-dev): Can this be template? std::vector InterpretercoreInferShapeContext::GetInputVarPtrs( const std::string& name) const { diff --git a/paddle/fluid/framework/new_executor/new_executor_defs.h b/paddle/fluid/framework/new_executor/new_executor_defs.h index ca49e7f5670d6..5d63eb33d424b 100644 --- a/paddle/fluid/framework/new_executor/new_executor_defs.h +++ b/paddle/fluid/framework/new_executor/new_executor_defs.h @@ -84,6 +84,8 @@ class InterpretercoreInferShapeContext : public InferShapeContext { bool IsRuntime() const override; + bool IsRunMKLDNNKernel() const override; + // TODO(paddle-dev): Can this be template? std::vector GetInputVarPtrs( const std::string& name) const override; diff --git a/paddle/fluid/framework/op_desc.cc b/paddle/fluid/framework/op_desc.cc index 4254ec236d473..7bceeb05bac59 100644 --- a/paddle/fluid/framework/op_desc.cc +++ b/paddle/fluid/framework/op_desc.cc @@ -240,6 +240,8 @@ class CompileTimeInferShapeContext : public InferShapeContext { bool IsRuntime() const override; + bool IsRunMKLDNNKernel() const override; + std::vector GetInputsVarType( const std::string &name) const override { return GetVarTypes(Inputs(name)); @@ -930,6 +932,8 @@ void CompileTimeInferShapeContext::SetRepeatedDims( bool CompileTimeInferShapeContext::IsRuntime() const { return false; } +bool CompileTimeInferShapeContext::IsRunMKLDNNKernel() const { return false; } + proto::VarType::Type CompileTimeInferShapeContext::GetVarType( const std::string &name) const { return block_.FindVarRecursive(name)->GetType(); diff --git a/paddle/fluid/framework/operator.cc b/paddle/fluid/framework/operator.cc index dc4d1365093aa..93349b8b88449 100644 --- a/paddle/fluid/framework/operator.cc +++ b/paddle/fluid/framework/operator.cc @@ -884,6 +884,17 @@ class RuntimeInferShapeContext : public InferShapeContext { bool IsRuntime() const override { return true; } + bool IsRunMKLDNNKernel() const override { + try { + auto& op_with_kernel = dynamic_cast(op_); + return ((op_with_kernel.kernel_type()) && + (op_with_kernel.kernel_type()->data_layout_ == + framework::DataLayout::kMKLDNN)); + } catch (std::bad_cast exp) { + return false; + } + } + // TODO(paddle-dev): Can this be template? std::vector GetInputVarPtrs( const std::string& name) const override { @@ -1178,9 +1189,7 @@ void OperatorWithKernel::RunImpl(const Scope& scope, platform::RecordEvent record_event("infer_shape", platform::EventRole::kInnerOp); RuntimeInferShapeContext infer_shape_ctx(*this, *runtime_ctx); - // TODO(chenweihang): replace this after removing `this->IsMKLDNNType()` - // in some mkldnn infershape functions, such conv2d infershape - this->InferShape(&infer_shape_ctx); + this->Info().infer_shape_(&infer_shape_ctx); } if (FLAGS_enable_unused_var_check) { diff --git a/paddle/fluid/framework/operator.h b/paddle/fluid/framework/operator.h index 09e4abc77f573..8e69f96dfb813 100644 --- a/paddle/fluid/framework/operator.h +++ b/paddle/fluid/framework/operator.h @@ -528,11 +528,6 @@ class OperatorWithKernel : public OperatorBase { return g_all_op_kernels; } - bool IsMKLDNNType() const { - return ((this->kernel_type_) && (this->kernel_type_->data_layout_ == - framework::DataLayout::kMKLDNN)); - } - bool SupportGPU() const override { auto& op_kernels = OperatorWithKernel::AllOpKernels().at(type_); return std::any_of(op_kernels.begin(), op_kernels.end(), @@ -609,6 +604,8 @@ class OperatorWithKernel : public OperatorBase { return pt_kernel_context_.get(); } + const OpKernelType* kernel_type() const { return kernel_type_.get(); } + private: void RunImpl(const Scope& scope, const platform::Place& place) const final; void RunImpl(const Scope& scope, const platform::Place& place, diff --git a/paddle/fluid/framework/shape_inference.h b/paddle/fluid/framework/shape_inference.h index 10b0fa6afd78a..791600b39c3d9 100644 --- a/paddle/fluid/framework/shape_inference.h +++ b/paddle/fluid/framework/shape_inference.h @@ -102,6 +102,8 @@ class InferShapeContext { virtual bool IsRuntime() const = 0; + virtual bool IsRunMKLDNNKernel() const = 0; + virtual std::vector GetInputVarPtrs( const std::string &name) const = 0; virtual std::vector GetOutputVarPtrs( diff --git a/paddle/fluid/imperative/infer_shape_context.h b/paddle/fluid/imperative/infer_shape_context.h index 167d5682cbfdb..a16ad1688fbac 100644 --- a/paddle/fluid/imperative/infer_shape_context.h +++ b/paddle/fluid/imperative/infer_shape_context.h @@ -32,16 +32,17 @@ class DygraphInferShapeContext : public framework::InferShapeContext { using DDim = framework::DDim; public: - DygraphInferShapeContext(const NameVarMap* in, - const NameVarMap* out, - const framework::AttributeMap* attr, - const framework::AttributeMap* default_attr, - const std::string op_type) + DygraphInferShapeContext( + const NameVarMap* in, const NameVarMap* out, + const framework::AttributeMap* attr, + const framework::AttributeMap* default_attr, const std::string op_type, + const framework::OpKernelType* op_kernel_type = nullptr) : var_base_map_in_(in), var_base_map_out_(out), attrs_(attr), default_attrs_(default_attr), - op_type_(op_type) {} + op_type_(op_type), + op_kernel_type_(op_kernel_type) {} bool HasInput(const std::string& name) const override { // has only one input @@ -214,6 +215,11 @@ class DygraphInferShapeContext : public framework::InferShapeContext { bool IsRuntime() const override { return true; } + bool IsRunMKLDNNKernel() const override { + return (op_kernel_type_ && + (op_kernel_type_->data_layout_ == framework::DataLayout::kMKLDNN)); + } + // TODO(paddle-dev): Can this be template? std::vector GetInputVarPtrs( const std::string& name) const override { @@ -399,6 +405,7 @@ class DygraphInferShapeContext : public framework::InferShapeContext { const framework::AttributeMap* attrs_; const framework::AttributeMap* default_attrs_; const std::string op_type_; + const framework::OpKernelType* op_kernel_type_; }; } // namespace imperative diff --git a/paddle/fluid/imperative/prepared_operator.cc b/paddle/fluid/imperative/prepared_operator.cc index 1d12ecf30ede5..46e974c8f43f3 100644 --- a/paddle/fluid/imperative/prepared_operator.cc +++ b/paddle/fluid/imperative/prepared_operator.cc @@ -514,8 +514,8 @@ static void PreparedOpRunImpl( // TODO(zjl): remove scope in dygraph framework::Scope scope; - DygraphInferShapeContext infer_shape_ctx(&ins, &outs, &attrs, - &default_attrs, op.Type()); + DygraphInferShapeContext infer_shape_ctx( + &ins, &outs, &attrs, &default_attrs, op.Type(), &kernel_type); op.Info().infer_shape_(&infer_shape_ctx); func(DygraphExecutionContext(op, scope, *dev_ctx, ctx, ins, outs, @@ -560,8 +560,8 @@ static void PreparedOpRunPtImpl( platform::DeviceContext* dev_ctx, const NameVarMap& ins, const NameVarMap& outs, const framework::AttributeMap& attrs, const framework::AttributeMap& default_attrs) { - DygraphInferShapeContext infer_shape_ctx(&ins, &outs, &attrs, - &default_attrs, op.Type()); + DygraphInferShapeContext infer_shape_ctx( + &ins, &outs, &attrs, &default_attrs, op.Type(), &kernel_type); op.Info().infer_shape_(&infer_shape_ctx); BuildDygraphPtenKernelContext(pt_kernel_signature, pt_kernel, ins, diff --git a/paddle/fluid/operators/batch_norm_op.cc b/paddle/fluid/operators/batch_norm_op.cc index bc5bd118dbec4..0a8e753c01dc0 100644 --- a/paddle/fluid/operators/batch_norm_op.cc +++ b/paddle/fluid/operators/batch_norm_op.cc @@ -93,7 +93,7 @@ void BatchNormOp::InferShape(framework::InferShapeContext *ctx) const { x_dims, x_dims.size())); const int64_t C = - ((this->IsMKLDNNType() == true) || (data_layout == DataLayout::kNCHW) + ((ctx->IsRunMKLDNNKernel() == true) || (data_layout == DataLayout::kNCHW) ? x_dims[1] : x_dims[x_dims.size() - 1]); @@ -508,7 +508,7 @@ void BatchNormGradOp::InferShape(framework::InferShapeContext *ctx) const { ctx->Attrs().Get("data_layout")); const int C = - ((this->IsMKLDNNType() == true) || (data_layout == DataLayout::kNCHW) + ((ctx->IsRunMKLDNNKernel() == true) || (data_layout == DataLayout::kNCHW) ? x_dims[1] : x_dims[x_dims.size() - 1]); @@ -911,7 +911,7 @@ void BatchNormDoubleGradOp::InferShape( const DataLayout data_layout = framework::StringToDataLayout( ctx->Attrs().Get("data_layout")); const int C = - ((this->IsMKLDNNType() == true) || (data_layout == DataLayout::kNCHW) + ((ctx->IsRunMKLDNNKernel() == true) || (data_layout == DataLayout::kNCHW) ? x_dims[1] : x_dims[x_dims.size() - 1]); diff --git a/paddle/fluid/operators/conv_op.cc b/paddle/fluid/operators/conv_op.cc index 41f6f75200697..e500814232aae 100644 --- a/paddle/fluid/operators/conv_op.cc +++ b/paddle/fluid/operators/conv_op.cc @@ -57,7 +57,7 @@ std::vector ConvOp::ComputeOutputShape( // MKL-DNN Kernels are using NCHW order of dims description // so we ignore data_format consideration for MKL-DNN kernel - const bool channel_last = (this->IsMKLDNNType() == false) && + const bool channel_last = (ctx->IsRunMKLDNNKernel() == false) && (data_format == "NHWC" || data_format == "NDHWC"); PADDLE_ENFORCE_EQ( diff --git a/paddle/fluid/operators/conv_transpose_op.cc b/paddle/fluid/operators/conv_transpose_op.cc index d60786f60e9cc..12f537e2f7980 100644 --- a/paddle/fluid/operators/conv_transpose_op.cc +++ b/paddle/fluid/operators/conv_transpose_op.cc @@ -49,8 +49,8 @@ void ConvTransposeOp::InferShape(framework::InferShapeContext* ctx) const { const std::string data_layout_str = ctx->Attrs().Get("data_format"); const DataLayout data_layout = - this->IsMKLDNNType() ? DataLayout::kNCHW - : framework::StringToDataLayout(data_layout_str); + ctx->IsRunMKLDNNKernel() ? DataLayout::kNCHW + : framework::StringToDataLayout(data_layout_str); PADDLE_ENFORCE_EQ(in_dims.size() == 4 || in_dims.size() == 5, true, platform::errors::InvalidArgument( diff --git a/paddle/fluid/operators/inplace_abn_op.cc b/paddle/fluid/operators/inplace_abn_op.cc index 8234d63d681ff..7a112292c8fc5 100644 --- a/paddle/fluid/operators/inplace_abn_op.cc +++ b/paddle/fluid/operators/inplace_abn_op.cc @@ -100,10 +100,10 @@ class InplaceABNGradOp : public paddle::operators::BatchNormGradOp { const DataLayout data_layout = framework::StringToDataLayout( ctx->Attrs().Get("data_layout")); - const int C = - ((this->IsMKLDNNType() == true) || (data_layout == DataLayout::kNCHW) - ? y_dims[1] - : y_dims[y_dims.size() - 1]); + const int C = ((ctx->IsRunMKLDNNKernel() == true) || + (data_layout == DataLayout::kNCHW) + ? y_dims[1] + : y_dims[y_dims.size() - 1]); ctx->SetOutputDim(framework::GradVarName("X"), y_dims); // has_scale_grad == has_bias_grad, judge has_scale_grad is enough diff --git a/paddle/fluid/operators/pool_op.cc b/paddle/fluid/operators/pool_op.cc index fa98e76e39338..b4ba80ae7ae2f 100644 --- a/paddle/fluid/operators/pool_op.cc +++ b/paddle/fluid/operators/pool_op.cc @@ -97,7 +97,7 @@ void PoolOp::InferShape(framework::InferShapeContext* ctx) const { // MKL-DNN Kernels are using NCHW order of dims description // so we ignore data_format consideration for MKL-DNN kernel - const bool channel_last = (this->IsMKLDNNType() == false) && + const bool channel_last = (ctx->IsRunMKLDNNKernel() == false) && (data_format == "NHWC" || data_format == "NDHWC"); // update paddings if "SAME" or global_pooling diff --git a/python/paddle/fluid/tests/unittests/mkldnn/test_conv2d_mkldnn_op.py b/python/paddle/fluid/tests/unittests/mkldnn/test_conv2d_mkldnn_op.py index 50d53864789f3..487a69807e2b0 100644 --- a/python/paddle/fluid/tests/unittests/mkldnn/test_conv2d_mkldnn_op.py +++ b/python/paddle/fluid/tests/unittests/mkldnn/test_conv2d_mkldnn_op.py @@ -14,6 +14,7 @@ from __future__ import print_function +import os import unittest import numpy as np @@ -232,6 +233,15 @@ def init_group(self): self.groups = 3 +# TODO(chenweihang): To solve the coverage problem, add this unittest, +# remove this unittest after new executor set to default executor +class TestConv2dMKLDNNByNewExecutor(TestConv2DMKLDNNOp): + def test_check_output_by_new_executor(self): + os.environ['FLAGS_USE_STANDALONE_EXECUTOR'] = '1' + self.test_check_output() + del os.environ['FLAGS_USE_STANDALONE_EXECUTOR'] + + if __name__ == '__main__': from paddle import enable_static enable_static()