You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2020/09/18 22:00:12 UTC
[GitHub] [incubator-tvm] rkimball opened a new pull request #6515: Generalize the use of booleans to support all cmake boolean values.
rkimball opened a new pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515
There are multiple instances where cmake variables are used as a combination boolean and/or path specifier. The code checks for a boolean by doing a string comparison with "ON" or "OFF" but cmake booleans can have many more values than these.
From the cmake docs:
`True if the constant is 1, ON, YES, TRUE, Y, or a non-zero number. False if the constant is 0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND. Named boolean constants are case-insensitive`
I added a new cmake function `decompose_args` which decomposes a cmake variable like USE_LLVM which could be a bool or an executable file into two cmake variables, a boolean and a `command`. This makes it easier to assemble the correct conditionals to use the values and also handles all valid boolean values for true and false.
It can now do things like:
```
cmake .. -DUSE_LLVM=1
cmake .. -DUSE_LLVM=llvm-config (if llvm-config is in the path)
cmake .. -DUSE_LLVM=/path/to/llvm-config
cmake .. -DUSE_LLVM=False
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r491218858
##########
File path: cmake/util/Util.cmake
##########
@@ -74,3 +74,31 @@ function(assign_source_group group)
source_group("${group}\\${_source_path_msvc}" FILES "${_source}")
endforeach()
endfunction(assign_source_group)
+
+cmake_policy(SET CMP0057 NEW) # Needed for IN_LIST used in conditional
Review comment:
see the regex example below
##########
File path: cmake/util/Util.cmake
##########
@@ -74,3 +74,31 @@ function(assign_source_group group)
source_group("${group}\\${_source_path_msvc}" FILES "${_source}")
endforeach()
endfunction(assign_source_group)
+
+cmake_policy(SET CMP0057 NEW) # Needed for IN_LIST used in conditional
Review comment:
this might come as a suprise to other cmake modules. would be great if we can avoid. e.g. we can write a few if after for instead since there are not a lot of cases
##########
File path: cmake/util/Util.cmake
##########
@@ -74,3 +74,31 @@ function(assign_source_group group)
source_group("${group}\\${_source_path_msvc}" FILES "${_source}")
endforeach()
endfunction(assign_source_group)
+
+cmake_policy(SET CMP0057 NEW) # Needed for IN_LIST used in conditional
+function(DECOMPOSE_ARG SOURCE_VALUE)
+ set(options)
+ set(oneValueArgs ENABLE_VALUE COMMAND_VALUE)
+ set(multiValueArgs)
+ cmake_parse_arguments(ARG "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
+ # First check if SOURCE_VALUE is executable
+ execute_process(COMMAND ${SOURCE_VALUE}
+ RESULT_VARIABLE exit_code
+ OUTPUT_VARIABLE dummy
+ OUTPUT_QUIET ERROR_QUIET)
+ string(TOUPPER ${SOURCE_VALUE} UPPER_CASE_SOURCE_VALUE)
+ set(FALSE_VALUES OFF 0 NO FALSE N IGNORE NOTFOUND)
+ if(${SOURCE_VALUE})
+ # If not executable, is it true
+ set(${ARG_ENABLE_VALUE} ON PARENT_SCOPE)
+ unset(${ARG_COMMAND_VALUE} PARENT_SCOPE)
+ elseif(${UPPER_CASE_SOURCE_VALUE} IN_LIST FALSE_VALUES)
Review comment:
perhaps we can do
if (value MATCHS "^(OFF|FALSE|0|N)$")
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r494432838
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
I had not made an IS_TRUE_PATTERN because `if(${VAR})` works in all cases, but I will add the pattern for symmetry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r494428369
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
For readability, let us do ${USE_CUDA} MATCHES ${IS_TRUE_PATTERN} :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r494428369
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
For readability, let us do ${USE_CUDA} MATCHES ${IS_TRUE_PATTERN} :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r492172680
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
This case does not need any special treatment, it is checking if __use_cuda is a boolean true value. If __use_cuda is a string (path) then this test will fail and the test on line 42 below is checked.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r493899130
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
For readability, let us do `${USE_CUDA} MATCHES ${IS_TRUE_PATTERN}` :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen merged pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen merged pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r494432838
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
I had not made an IS_TRUE_PATTERN because `if(${VAR})` works in all cases, but I will add the pattern for symmetry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] rkimball commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
rkimball commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r492172680
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
This case does not need any special treatment, it is checking if __use_cuda is a boolean true value. If __use_cuda is a string (path) then this test will fail and the test on line 42 below is checked.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on a change in pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#discussion_r491217782
##########
File path: cmake/util/FindCUDA.cmake
##########
@@ -37,7 +37,7 @@
#
macro(find_cuda use_cuda)
set(__use_cuda ${use_cuda})
- if(__use_cuda STREQUAL "ON")
+ if(${__use_cuda})
Review comment:
we should call decompose_arg on this case as well. Most of the command will require decompose_arg
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-tvm] tqchen commented on pull request #6515: Generalize the use of booleans to support all cmake boolean values.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #6515:
URL: https://github.com/apache/incubator-tvm/pull/6515#issuecomment-697942835
@rkimball please look into CI errors
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org