Skip to content
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

[CustomOp] Support to specific extra_cflags and exctra_cuda_flags independently #31059

Merged
merged 14 commits into from
Feb 24, 2021

Conversation

Aurelius84
Copy link
Contributor

@Aurelius84 Aurelius84 commented Feb 19, 2021

PR types

New features

PR changes

APIs

Describe

What's New?

  • Support to specific extra_cflags and exctra_cuda_flags independently.
  • Enhance checking name argument in setup API and not allow using "module" as suffix in name .

@paddle-bot-old
Copy link

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

Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

LGTM, can polish details in next PR

@@ -561,14 +555,14 @@ def load(name,
Args:
name(str): generated shared library file name.
sources(list[str]): custom op source files name with .cc/.cu suffix.
extra_cflag(list[str]): additional flags used to compile CPP files. By default
extra_cxx_cflags(list[str]): additional flags used to compile CPP files. By default
Copy link
Contributor

Choose a reason for hiding this comment

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

extra_cxx_cflags(list[str], optional), others same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在英文文档的PR里会统一修改

@@ -30,7 +30,8 @@
name='dispatch_op',
sources=['dispatch_test_op.cc'],
extra_include_paths=paddle_includes, # add for Coverage CI
extra_cflags=extra_compile_args, # add for Coverage CI
extra_cxx_cflags=extra_compile_args,
extra_cuda_cflags=extra_compile_args, # add for Coverage CI
Copy link
Contributor

Choose a reason for hiding this comment

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

only cc test may not need extra_cuda_cflags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在英文文档的PR里会统一修改

@@ -34,7 +34,8 @@
name='multi_out_jit',
sources=['multi_out_test_op.cc'],
extra_include_paths=paddle_includes, # add for Coverage CI
extra_cflags=extra_compile_args, # add for Coverage CI
extra_cxx_cflags=extra_compile_args, # add for Coverage CI
extra_cuda_cflags=extra_compile_args, # add for Coverage CI
Copy link
Contributor

Choose a reason for hiding this comment

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

same above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我在英文文档的PR里会统一修改

Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

LGTM

@Aurelius84 Aurelius84 merged commit 406f4a7 into PaddlePaddle:develop Feb 24, 2021
chenwhql pushed a commit to chenwhql/Paddle that referenced this pull request Feb 26, 2021
…ependently (PaddlePaddle#31059)

* split cxx/nvcc compile flags

* enhance input argument check

* rename extra_cflags into extrac_cxx_flags

* add name checking in setup

* fix test_dispatch failed

* fix word typo and rm usless import statement

* refine import statement

* fix unittest failed

* fix cuda flags error
@Aurelius84 Aurelius84 deleted the custom_op_flags branch April 12, 2021 03:36
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.

3 participants