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

Fix ninja compilation bug and warning on windows #32987

Merged
merged 4 commits into from
May 26, 2021

Conversation

zhwesky2010
Copy link
Contributor

@zhwesky2010 zhwesky2010 commented May 19, 2021

PR types

Others

PR changes

Others

Describe

1.fix ninja compilation MSVC(/MT and /MD flags) bug.

image

2.fix ninja compilation warning on windows.

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.

@zhwesky2010 zhwesky2010 changed the title fix incremental compilation on windows fix ninja compilation bug and warning on windows May 21, 2021
@zhwesky2010 zhwesky2010 force-pushed the fix_windows_ci3 branch 2 times, most recently from 0bba9af to e8e05d4 Compare May 21, 2021 10:56
@zhwesky2010 zhwesky2010 changed the title fix ninja compilation bug and warning on windows Fix ninja compilation bug and warning on windows May 25, 2021
"Choose the type of build, options are: Debug Release RelWithDebInfo MinSizeRel"
FORCE)
endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

project前后写有啥区别吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

写的靠后无法生效,Ninja在project后设置CMAKE_BUILD_TYPE为Debug,导致Release的默认参数失效

BUILD_ALWAYS 1
# UPDATE_COMMAND ""
UPDATE_COMMAND ""
#BUILD_ALWAYS 1
Copy link
Contributor

Choose a reason for hiding this comment

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

这个对Ninja构建有什么影响呢?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不影响Ninja,这个BUILD_ALWAYS导致mkldnn、warpctc增量编译失效,0改动完全重编

PATCH_COMMAND ""
BUILD_ALWAYS 1
#BUILD_ALWAYS 1
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

Copy link
Contributor Author

Choose a reason for hiding this comment

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

同mkldnn

if(NOT WIN32)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14")
else()
set(CMAKE_CXX_STANDARD 14)
Copy link
Contributor

Choose a reason for hiding this comment

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

set(CMAKE_CXX_STANDARD 14)之后,在linux、mac上是不是也不用再单独设置CMAKE_CXX_FLAGS了?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linux mac下CMAKE_CXX_STANDARD=14有编译wanring,没有改动,
VS下-std=c++14没有生效,会屏蔽这个flag,所以也没warning,但Ninja不会屏蔽,导致所有cl报warning

@@ -412,10 +412,10 @@ if "%WITH_TESTING%"=="ON" (

echo Build Paddle the %build_times% time:
if %GENERATOR% == "Ninja" (
ninja -j %PARALLEL_PROJECT_COUNT%
ninja all
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的不指定-j会不会造成并行过高而影响编译时间?

Copy link
Contributor Author

@zhwesky2010 zhwesky2010 May 25, 2021

Choose a reason for hiding this comment

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

应该不会,之前是由于/m 和/MP 累乘,导致编译进程有核数的平方,而ninja默认并行思路就是核数大小,降了可能会变慢,先用默认的看看

Avin0323
Avin0323 previously approved these changes May 25, 2021
Copy link
Contributor

@Avin0323 Avin0323 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@wanghuancoder wanghuancoder left a comment

Choose a reason for hiding this comment

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

LGTM

@zhwesky2010 zhwesky2010 merged commit accf284 into PaddlePaddle:develop May 26, 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.

3 participants