Profiling - CMake by philippefoubert · Pull Request #17678 · opencv/opencv

philippefoubert · 2020-06-27T10:18:08Z

resolves #17657

cmake/OpenCVUtils.cmake

mshabunin · 2020-06-29T14:15:51Z

cmake/OpenCVUtils.cmake

+# - 'TEST_FILE <c1> <c2>'              - test file
+# - 'TURN_OFF_COMPILER_FLAG <d1> <d2>' - incompatible options that shall be turned off
+# - 'TURN_OFF_LINKER_FLAG <e1> <e2>'   - incompatible options that shall be turned off
+# - 'LINKER_FLAG <f1> <f2>'            - linker flag that shall be used with the compiler flag


I'd suggest rewriting help string in more traditional cmake format:

ocv_check_compiler_flag(<LANG> <FLAG> <RESULT> [UPDATED_COMPILER_FLAGS <var>] [UPDATED_LINKER_FLAGS <var>] [TEST_FILE <file>] [TURN_OFF_COMPILER_FLAG <flag> [<flag> ...]] [TURN_OFF_LINKER_FLAG <flag> [<flag> ...]] [LINKER_FLAG <flag> [<flag> ...]])

I don't think it is a good idea to mix variable update functionality to this macro. It adds unnecessary complexity and does not correlate with macro name: ocv_check_compiler_flag. "check" usually means that nothing will be changed and macro will just return some value.

I agree but i had to deal with its usage: this macro is not always used the same variable containing the compiler/linker flags. Would you find better to pass these variables in read only and modifying them, if necessary, outside of the macro?

Why not add these options to add_extra_compiler_option macro? Or a new higher level macro/function which will be used in one or two places?

Why haven't you make a patch that you proposed in the issue? It was much simpler.

cmake/OpenCVCompilerOptions.cmake

* Ampere has CC 8.0 * Arm64 server support has been added in CUDA 11 (only V100 for now)

* use only supported CC in the list * workaround of opencv#17526

origibal commit: 63e92cc

http://www.ijg.org/files/jpegsrc.v9d.tar.gz

philippefoubert · 2020-06-30T10:52:39Z

Can the build be relaunch once more? The problem (usage of "list(join ...)" not supported if CMake is not up to date) should be solved.

cmake/OpenCVUtils.cmake

…ions, add dependant linker flags, etc.)

philippefoubert · 2020-06-30T21:10:26Z

My new version is much better but the builds fail because the LINK_OPTIONS option of try_compile() does not exist before CMake 3.14. Do you have any workaround to suggest?

…e <3.14

asmorkalov · 2020-07-15T09:52:54Z

@philippefoubert @mshabunin friendly reminder.

philippefoubert · 2020-07-19T18:58:14Z

@philippefoubert @mshabunin friendly reminder.

I was off work for a few days... 😏
I think now the PR is ready but, since flags have changed, a manual validation is required.

asmorkalov · 2020-07-29T06:21:14Z

@mshabunin Friendly reminder.

philippefoubert · 2020-08-25T19:02:54Z

@asmorkalov @mshabunin Did you have time to perform the code review?

mshabunin · 2020-08-26T09:46:05Z

@philippefoubert , looks like compile options has changed:

  Actual (486): '-fvisibility-inlines-hidden -fdata-sections
  -ffunction-sections -fomit-frame-pointer -pthread -Wno-long-long
  -fdiagnostics-show-option -Wno-missing-field-initializers -Wno-comment
  -Wno-delete-non-virtual-dtor -Winit-self -Wuninitialized -Wsign-promo
  -Wshadow -Wpointer-arith -Winit-self -Wundef -Wmissing-declarations
  -Werror=format-security -Wformat -Wformat -Werror=sequence-point
  -Werror=address -Werror=non-virtual-dtor -Werror=return-type -Wall -W
  -fsigned-char -msse -msse2 -msse3'

  Expected (497): '-fsigned-char -W -Wall -Werror=return-type
  -Werror=non-virtual-dtor -Werror=address -Werror=sequence-point -Wformat
  -Werror=format-security -Wmissing-declarations -Wundef -Winit-self
  -Wpointer-arith -Wshadow -Wsign-promo -Wuninitialized -Winit-self
  -Wno-delete-non-virtual-dtor -Wno-comment -Wno-missing-field-initializers
  -fdiagnostics-show-option -Wno-long-long -pthread -fomit-frame-pointer
  -ffunction-sections -fdata-sections -msse -msse2 -msse3 -fvisibility=hidden
  -fvisibility-inlines-hidden'

I can see duplicated -Wformat option and wrong order: -Wall -W - moved to the end.

I thought the issue with ENABLE_PROFILING have been fixed in #17744, haven't it?

alalek · 2020-08-26T09:30:43Z

cmake/OpenCVCompilerOptions.cmake


+# add_extra_compiler_option(<option> [...]
+#                           [TURN_OFF_COMPILER_FLAG <flag> [<flag> ...]]
+#                           [TURN_OFF_LINKER_FLAG <flag> [<flag> ...]]


TURN_OFF

Perhaps naming is not accurate. Suggested variants:

REMOVE

or EXCLUDE

Also it would be nice to allow patterns here.

I have made the renaming. Could explain what you think about when you are talking about patterns ?

I mean to replace checks if(... STREQUAL ...) => if(... MATHES "${regex_pattern}")

In this case use string(REGEX REPLACE ...) instead of string(REPLACE ...) (but need to add argument boundary checks)

Mostly needed for handling of non-boolean options with "values", like -march= or -O2 / -O1 / -O0 / -O3 / Os.

alalek · 2020-08-26T09:32:57Z

cmake/OpenCVCompilerOptions.cmake

+  add_extra_compiler_option("-Wformat")
+  add_extra_compiler_option("-Werror=format-security -Wformat")
+  add_extra_compiler_option("-Wmissing-declarations")
+  add_extra_compiler_option("-Wmissing-prototypes")


Do we really need such massive changes?

BTW, good PR should not break existed code and doesn't require massive changes.

I have only added the double quotes to ease the parsing in the macro and increase homogeneity: some of the calls to add_extra_compiler_option were done with double quotes and some were done without double quotes.

cmake/OpenCVCompilerOptions.cmake

alalek · 2020-08-26T09:36:11Z

cmake/OpenCVUtils.cmake

+        message(STATUS "Performing Test ${RESULT} - Success")
+      else(${RESULT})
+        message(STATUS "Performing Test ${RESULT} - Failed")
+        # message("${OUTPUT}")


remove unnecessary debug code (it is already added to CMakeError.log)

@mshabunin > I thought the issue with ENABLE_PROFILING have been fixed in #17744, haven't it?

The #17744 is a quick fix to the ENABLE_PROFILING while the #17678 is more generic: the incompatibility between compiler options is not only related to profiling.

Please keep original message(STATUS "Performing Test ${RESULT} - Failed") messages.

alalek · 2020-08-26T09:40:45Z

cmake/OpenCVCompilerOptimizations.cmake

          set(_varname "HAVE_CPU_${OPT}_SUPPORT")
-          ocv_check_compiler_flag(CXX "${CPU_BASELINE_FLAGS}" "${_varname}" "${CPU_${OPT}_TEST_FILE}")
+          ocv_check_compiler_flag(CXX "${CPU_BASELINE_FLAGS}" "${_varname}"
+                                  LINKER_FLAGS "${OPENCV_EXTRA_EXE_LINKER_FLAGS}"


OPENCV_EXTRA_EXE_LINKER_FLAGS

IMHO, this should be assumed in "ocv_" check function.

cmake/OpenCVUtils.cmake

…o EXCLUDE_xxx

…ated to "-fvisibility=hidden", integration

philippefoubert · 2020-08-29T18:44:38Z

📌 (not directly related to this PR):
Following gcc: error: long: No such file or directory, i have to set OPENCV_ENABLE_ALLOCATOR_STATS=OFF to be able to compile using mingw32

alalek · 2020-09-03T18:07:28Z

@philippefoubert Related issue is here: #16990
You may want to provide automatic handling for MinGW in CMake scripts and replace "long long" with "int64_t", see modules/core/CMakeLists.txt. See #16990 (comment)

asmorkalov · 2020-11-26T08:15:27Z

@mshabunin @philippefoubert Friendly reminder. The PR still does not pass CI checks.

asmorkalov · 2021-01-28T07:57:33Z

@mshabunin @philippefoubert Friendly reminder. The PR still does not pass CI checks.

asenyaev · 2021-04-08T21:50:24Z

jenkins cn please retry a build

mshabunin · 2023-11-21T20:22:34Z

This PR has been superseded by #17744.

mshabunin reviewed Jun 29, 2020

View reviewed changes

philippefoubert force-pushed the enable_profiling branch from 972af2c to acb4025 Compare June 29, 2020 19:06

tomoaki0705 and others added 7 commits June 29, 2020 19:28

add Ampere CC

e55b8ab

* Ampere has CC 8.0 * Arm64 server support has been added in CUDA 11 (only V100 for now)

make the fail safe path actually safe

6d3cf30

* use only supported CC in the list * workaround of opencv#17526

Disabling dafault NMS in yolo layer

3fd5e74

Remove deprecated Inference Engine CPU extensions

7da51f7

Conditional compilation for network reader

7142ece

origibal commit: 63e92cc

Optimize Mish for CPU backend

d7bb5c7

3rdparty: libjpeg 9d

2278591

http://www.ijg.org/files/jpegsrc.v9d.tar.gz

philippefoubert force-pushed the enable_profiling branch from acb4025 to 760e509 Compare June 29, 2020 19:29

mshabunin reviewed Jun 30, 2020

View reviewed changes

cmake/OpenCVUtils.cmake Outdated Show resolved Hide resolved

philippefoubert force-pushed the enable_profiling branch from 760e509 to caa5cfe Compare June 30, 2020 20:39

Fix the usage of "ocv_check_compiler_flag" (turn off incompatible opt…

b0a0b68

…ions, add dependant linker flags, etc.)

philippefoubert force-pushed the enable_profiling branch from caa5cfe to b0a0b68 Compare June 30, 2020 20:48

philippefoubert added 3 commits July 3, 2020 19:21

Use "LINK_LIBRARIES" instead of "LINK_OPTIONS" to be able to use cmak…

5482c28

…e <3.14

Fix misprints

25db00e

Make "OPENCV_WARNINGS_ARE_ERRORS" the last option tested

29466b2

philippefoubert mentioned this pull request Jul 3, 2020

cmake: fix ENABLE_PROFILING #17744

Merged

Merge branch '3.4' into enable_profiling

a406d93

philippefoubert added 2 commits July 19, 2020 20:01

Fix a bug (not seen on buildbot)

0b068ac

Fix a misprint

25cf05c

alalek reviewed Aug 26, 2020

View reviewed changes

Fix the order of compiler options + Rename variables TURN_OFF_xxx int…

3536272

…o EXCLUDE_xxx

Code review: CMP0056, removal of debugging messages, fix the test rel…

606d44c

…ated to "-fvisibility=hidden", integration

mshabunin closed this Nov 21, 2023

Uh oh!

Conversation

philippefoubert commented Jun 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philippefoubert commented Jun 30, 2020

Uh oh!

Uh oh!

philippefoubert commented Jun 30, 2020

Uh oh!

asmorkalov commented Jul 15, 2020

Uh oh!

philippefoubert commented Jul 19, 2020

Uh oh!

asmorkalov commented Jul 29, 2020

Uh oh!

philippefoubert commented Aug 25, 2020

Uh oh!

mshabunin commented Aug 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

philippefoubert commented Aug 29, 2020

Uh oh!

alalek commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asmorkalov commented Nov 26, 2020

Uh oh!

asmorkalov commented Jan 28, 2021

Uh oh!

asenyaev commented Apr 8, 2021

Uh oh!

mshabunin commented Nov 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

alalek Sep 1, 2020 •

edited

Loading

alalek commented Sep 3, 2020 •

edited

Loading