emmenlau commented on code in PR #2780:
URL: https://github.com/apache/thrift/pull/2780#discussion_r1707174715
##########
lib/cpp/CMakeLists.txt:
##########
@@ -124,6 +124,58 @@ if(UNIX)
endif()
endif()
+if (CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)")
Review Comment:
Could you kindly change the indenting to 4 white spaces, instead of two?
##########
lib/cpp/CMakeLists.txt:
##########
@@ -124,6 +124,58 @@ if(UNIX)
endif()
endif()
+if (CMAKE_SYSTEM_PROCESSOR MATCHES "(x86)|(X86)|(amd64)|(AMD64)")
+ set(PREV_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
+ set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -mbmi2 -mbmi -mlzcnt
-msse3 -mavx512bw -mavx512vl")
Review Comment:
Questions:
- Not sure if these flags are portable to MSVC? On which platforms did you
test?
- Do you not enforce here already that the CPU supports all these flags?
Then, further down, you test which CPU features are supported? Am I missing
something?
Problems:
- Not all users build for their current platforms. I.e. people may want to
build on a modern CPU, but may plan to ship their binaries to broader
platforms. This is for example the case with Linux distributors. It may be
required to make these changes optional, particularly for the very advanced
features. I.e. building on an AVX512 capable CPU should not necessarily make
the binaries non-portable to non-AVX512-CPUs (which still exist quite a lot).
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
