Skip to content

[CMake] Handle clang in MSVC mode in GetHostTriple #116701

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.

Sign up for GitHub

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

Merged
DanielKristofKiss merged 4 commits into from
Nov 29, 2024
Merged

[CMake] Handle clang in MSVC mode in GetHostTriple #116701

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38d6e3f
Build clang with the latest released clang on Windows on Arm.
DanielKristofKiss Nov 18, 2024
48a57b2
simplify the controlflow
DanielKristofKiss Nov 19, 2024
480feee
s/CMAKE_HOST_SYSTEM_NAME STREQUAL/CMAKE_SYSTEM_NAME
DanielKristofKiss Nov 21, 2024
4653a11
add NOT MINGW AND NOT MSYS
DanielKristofKiss Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion llvm/cmake/modules/GetHostTriple.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,21 @@ function( get_host_triple var )
set( value "powerpc-ibm-aix" )
endif()
else( MSVC )
if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS)
if(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS AND CMAKE_C_COMPILER_ID MATCHES "Clang" )
Copy link
Member

@mstorsjo mstorsjo Nov 19, 2024

Choose a reason for hiding this comment

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

Why is this needed here? When operating in MSVC mode, CMake usually sets the MSVC CMake variable, so this should hit the existing codepath at lines 5-18 above? And this is a bit weird, because CMake usually doesn't set COMPILER_ARCHITECTURE_ID for Clang (or GCC), but only for MSVC. I guess CMake might set COMPILER_ARCHITECTURE_ID for Clang-cl when operating in MSVC mode, but if we're invoking it as clang, not clang-cl so that the MSVC variable isn't set, it feels odd that CMake still would be setting COMPILER_ARCHITECTURE_ID.

Also I don't quite see why this needs to be as a separate step within the else() clause here, why can't this be a toplevel case like the other ones?

The condition, CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS also feels quite contrieved for this case. I guess what you want to detect is "clang, targeting windows, probably in msvc mode (as we're past a check for MINGW further up)", and we generally don't want to check for the host unless we need to.

The existing check for CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS is to pick out configurations where we can't run the generic detection shell script below.

Can't we just extend the existing MSVC case at the top, e.g. something like if (MSVC OR (CMAKE_C_COMPILER_ID MATCHES "Clang" AND WIN32 AND CMAKE_C_COMPILER_ARCHITECTURE_ID) or something along those lines, to specifically match the case we're looking for here?

if( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*" )
set( value "aarch64-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*" )
set( value "armv7-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64" )
set( value "x86_64-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86" )
set( value "i686-pc-windows-msvc" )
elseif( CMAKE_SIZEOF_VOID_P EQUAL 8 )
set( value "x86_64-pc-windows-msvc" )
else()
set( value "i686-pc-windows-msvc" )
Copy link
Member

@petrhosek petrhosek Nov 19, 2024

Choose a reason for hiding this comment

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

We usually don't use spaces around parens.

Suggested change
if( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*" )
set( value "aarch64-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*" )
set( value "armv7-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64" )
set( value "x86_64-pc-windows-msvc" )
elseif( CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86" )
set( value "i686-pc-windows-msvc" )
elseif( CMAKE_SIZEOF_VOID_P EQUAL 8 )
set( value "x86_64-pc-windows-msvc" )
else()
set( value "i686-pc-windows-msvc" )
if(CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM64.*")
set(value "aarch64-pc-windows-msvc")
elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID MATCHES "ARM.*")
set(value "armv7-pc-windows-msvc")
elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "x64")
set(value "x86_64-pc-windows-msvc")
elseif(CMAKE_C_COMPILER_ARCHITECTURE_ID STREQUAL "X86")
set(value "i686-pc-windows-msvc")
elseif(CMAKE_SIZEOF_VOID_P EQUAL 8)
set(value "x86_64-pc-windows-msvc")
else()
set(value "i686-pc-windows-msvc")

Copy link
Member

@petrhosek petrhosek Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is it safe to assume x86/x64 when CMAKE_C_COMPILER_ARCHITECTURE_ID isn't x86 or x64? Can we report an error instead?

endif()
elseif(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows AND NOT MSYS)
message(WARNING "unable to determine host target triple")
else()
set(config_guess ${LLVM_MAIN_SRC_DIR}/cmake/config.guess)
Expand Down
Loading