Skip to content

cinn_launch_op: skip checking input variables must be used #37119

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

Merged

Conversation

CtfGo
Copy link
Contributor

@CtfGo CtfGo commented Nov 11, 2021

PR types

Function optimization

PR changes

OPs

Describe

Modify serveral implements on CinnLaunchOp:

  1. Skip checking input variables must be used
  2. Move current helper functions to a CinnlaunchContext

Run cinn_launch_op_test result:
image

Run test_resnet50_with_cinn.py result:
command: FLAGS_allow_cinn_ops="elementwise_add;elementwise_add_grad;relu;relu_grad;sum;" python test_resnet50_with_cinn.py

image
image
image
image

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -171,6 +171,7 @@ endif()
if (WITH_CINN)
op_library(cinn_launch_op SRCS cinn_launch_op.cc cinn_launch_op.cu.cc DEPS transform_desc cinn_compiler cinn ${OP_HEADER_DEPS})
cc_test(cinn_launch_op_test SRCS cinn_launch_op_test.cc DEPS cinn_compiler cinn_launch_op elementwise_add_op)
set_tests_properties(cinn_launch_op_test PROPERTIES ENVIRONMENT OMP_NUM_THREADS=1)
Copy link
Member

Choose a reason for hiding this comment

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

This comment is FYI (for you information). Currently, this test failed in Paddle-CINN CI due to libcublas version. Thank you for not setting it as RUN_TYPE=CINN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

const LoDTensor* paddle_tensor,
const CinnTensor& cinn_tensor) {
void CinnLaunchContext::CheckTensorEquivalent(const std::string& paddle_name,
const LoDTensor* paddle_tensor,
Copy link
Member

Choose a reason for hiding this comment

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

Paddle usually make non-null parameter as const reference instead of const pointer. Could you change the const LoDTensor* paddle_tensor to const LoDTensor& paddle_tensor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it is a good suggestion, thank you


// check dimension
auto cinn_dims = framework::make_ddim(cinn_tensor->shape().data());
PADDLE_ENFORCE_EQ(paddle_tensor->dims(), cinn_dims,
platform::errors::InvalidArgument(
"The tensor dimension in variable(%s) "
"is not equivalent, paddle is [%s] "
"Tensors dimension in variable(%s) "
Copy link
Member

Choose a reason for hiding this comment

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

NIT (Not important): I saw you noticed the English grammar error, but the change is not correct ...

Tensors' dimensions or Dimensions of tensors. The word "Dimensions" is the noun here and the verb is "are"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

[this](const auto& var_name) {
PADDLE_ENFORCE_GT(name2argument_.count(var_name), 0,
platform::errors::InvalidArgument(
"Variable(%s) is missed for launch "
Copy link
Member

Choose a reason for hiding this comment

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

Not Important: for launching

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

hold_buffers.emplace_back(std::move(buffer));
for (const auto& var_name : input_variable_names) {
if (!launch_context->IsVariableUsed(var_name)) {
// some input variables doesn't need for cinn because they are
Copy link
Member

Choose a reason for hiding this comment

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

"some input variables don't"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

const std::map<std::string, cinn_pod_value_t>&
CinnLaunchContext::FinalizeArguments() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

改成下面的函数名如何?

Suggested change
CinnLaunchContext::FinalizeArguments() const {
CinnLaunchContext::CheckValidArguments() const {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个函数需要返回最终参数,感觉直接叫check不合适,所以先不改了

@zhhsplendid zhhsplendid merged commit 228eb89 into PaddlePaddle:develop Nov 13, 2021
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.

4 participants