[FLANG] Fix MSVC + clang-cl build

[omjavaid]

Summary

Flang build on windows with MSVC environment and clang-cl compiler
requires clang_rt.builtin.${target} library. This patch allows us to
locate and include this link library. This is mostly needed for flang
runtime and associated unittests as it requires the uint128 division
builtin function __udivti3.

[mstorsjo]

Flang build on windows with MSVC environment and clang-cl compiler requires clang_rt.builtin.${target} library.

Can you elaborate on which parts ends up requiring it? The main case I know of is for 128 bit integer support routines.

Clang implicitly uses this library while calling lld-link via clang driver

This doesn't match my experience - the builtin library isn't included automatically anywhere. If you do link with -rtlib=compiler-rt, then yes, it will try to link it in though.

[omjavaid]

In D138023#3927238, @mstorsjo wrote:

Flang build on windows with MSVC environment and clang-cl compiler requires clang_rt.builtin.${target} library.

Can you elaborate on which parts ends up requiring it? The main case I know of is for 128 bit integer support routines.

Clang implicitly uses this library while calling lld-link via clang driver

This doesn't match my experience - the builtin library isn't included automatically anywhere. If you do link with -rtlib=compiler-rt, then yes, it will try to link it in though.

clang-cl implicitly uses clang_rt.builtins* but lld-link fails to include it. This fails a couple of flang unit tests and linking of bbc.exe with following error:

lld-link: error: undefined symbol: __udivti3

Building on Surface X Pro LLVM 15.0.1 release. Environtment set via "c:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\Build\vcvarsamd64_arm64.bat"

[mstorsjo]

In D138023#3927276, @omjavaid wrote:

clang-cl implicitly uses clang_rt.builtins*

Can you give an example of a concrete, minimal, clang-cl command line, where you run it with the -v option, where you show where clang-cl actually implicitly links in clang_rt.builtins, as you claim it does in some cases? Because in my experience, it never does that implicitly.

(This is of course not entirely on point for the patch itself, but I would like that the patch background represents facts precisely.)

In D138023#3927276, @omjavaid wrote:

lld-link: error: undefined symbol: __udivti3

Right, that's the uint128 division function. It would be valuable to include this information in the commit description.

Note that there have been many discussions about how to fix this issue properly in one way or another - see most recently https://reviews.llvm.org/D134912#3828421. For now, the conclusion within e.g. libcxx is to just avoid int128 code until this is fixed properly in clang-cl.

How does flang work when built with MSVC proper, which doesn't have int128 as a built-in type at all?

flang/CMakeLists.txt

44

Won't this try to find and use the compiler-rt library even in a full pure-MSVC build (i.e. no clang-cl)?

[omjavaid]

In D138023#3927298, @mstorsjo wrote:

In D138023#3927276, @omjavaid wrote:

clang-cl implicitly uses clang_rt.builtins*

Can you give an example of a concrete, minimal, clang-cl command line, where you run it with the -v option, where you show where clang-cl actually implicitly links in clang_rt.builtins, as you claim it does in some cases? Because in my experience, it never does that implicitly.

(This is of course not entirely on point for the patch itself, but I would like that the patch background represents facts precisely.)

So I went ahead and tried a simple int128 div and compiled it with clang-cl on Surface X. As you suggested clang does not implicitly link rt library. I actually just read docs that target specific rtlib is implicitly linked
https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst

In D138023#3927276, @omjavaid wrote:

lld-link: error: undefined symbol: __udivti3

Right, that's the uint128 division function. It would be valuable to include this information in the commit description.

Ack

Note that there have been many discussions about how to fix this issue properly in one way or another - see most recently https://reviews.llvm.org/D134912#3828421. For now, the conclusion within e.g. libcxx is to just avoid int128 code until this is fixed properly in clang-cl.

How does flang work when built with MSVC proper, which doesn't have int128 as a built-in type at all?

We have not been using cl as they are available as emulated tools on WoA while clang-cl is native and faster. Although I tried cl just today but was not able to get the build working facing errors which are unrelated to this particular issue.

[mstorsjo]

In D138023#3930101, @omjavaid wrote:

So I went ahead and tried a simple int128 div and compiled it with clang-cl on Surface X. As you suggested clang does not implicitly link rt library. I actually just read docs that target specific rtlib is implicitly linked
https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst

Ok, I see! Yes, that document might not take all clang-cl considerations into account.

For most clang targets, you really do need the builtins (either in the form of libgcc, or compiler-rt builtins) everywhere. But for MSVC targets, it's usually not needed and thus not linked in implicitly, since everything you normally need is in the MSVC static libraries. But since MSVC doesn't support int128 (and Clang lowers it to a libgcc named helper function) they obviously don't provide that one. (There has been discussions about whether they could start including these helpe functions somewhere, to help Clang cases, but it hasn't materialized yet at least.)

Overall, this is a Clang-cl toolchain issue - known for some time already; since this is a builtin type, the helpers for it should really be handled automatically. But since most use in MSVC style settings invoke the linker directly instead of via the compiler driver, the fix isn't entirely trivial. https://reviews.llvm.org/D134912#3828421 is the latest word on what would be a good way forward to handle it (from the toolchain point of view).

Also from a toolchain point of view, this is a bit tricky, since you normally can cross compile with clang-cl for any supported architecture as long as you've got MSVC and WinSDK, but when clang starts requiring the builtins, they you also need to have those built and available, for all architectures that you want to compile for. MSVC itself actually does ship with these files for some architectures (x86 and x64), but not for arm64 (or arm). So I think we should request that MSVC should start bundling clang_rt.*.lib for arm64 too, at least as many parts of compiler-rt as works on arm64 (IIRC the sanitizers don't support it yet), so that Clang could start automatically linking against it when needed (without needing to ship it along with Clang specifically for arm64 only).

In D138023#3927276, @omjavaid wrote:

How does flang work when built with MSVC proper, which doesn't have int128 as a built-in type at all?

We have not been using cl as they are available as emulated tools on WoA while clang-cl is native and faster. Although I tried cl just today but was not able to get the build working facing errors which are unrelated to this particular issue.

Ok, I see. (BTW since some months, MSVC should be available as properly native arm64 apps now too. But I don't expect you to try to set it up right now, I'm sure there's lots of things one would run into trying to build flang with it.)

I wonder why this isn't an issue with clang-cl builds on x86_64 too - the same issue should appear there too, if it uses int128 in the same way.

If it is possible to just decide not to use int128 for this build configuration, that's clearly the simplest solution. But if that's not possible, I guess something like this has to be done.

flang/CMakeLists.txt

44

When reading the cmake code more, yes I think this should have an additional check like if (MSVC AND CMAKE_CXX_COMPILER_ID MATCHES Clang).

[omjavaid]

Updated patch to include check for MSVC + Clang.
Also updated commit log description with information on uint128 division builtin function dependency.

[mstorsjo]

In D138023#3936235, @omjavaid wrote:

Updated patch to include check for MSVC + Clang.
Also updated commit log description with information on uint128 division builtin function dependency.

Thanks!

It seems like you got some unrelated lld changes included in the diff though...

I think this probably looks tolerable now, but it would be very interesting to know if the use of uint128 is isolated somehow, so that there's some conditional that can be set to opt out from it - I would expect this to exist somewhere, unless Flang doesn't support being built by pure MSVC at all?

[omjavaid]

In D138023#3930162, @mstorsjo wrote:

In D138023#3930101, @omjavaid wrote:

So I went ahead and tried a simple int128 div and compiled it with clang-cl on Surface X. As you suggested clang does not implicitly link rt library. I actually just read docs that target specific rtlib is implicitly linked
https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst

Ok, I see! Yes, that document might not take all clang-cl considerations into account.

For most clang targets, you really do need the builtins (either in the form of libgcc, or compiler-rt builtins) everywhere. But for MSVC targets, it's usually not needed and thus not linked in implicitly, since everything you normally need is in the MSVC static libraries. But since MSVC doesn't support int128 (and Clang lowers it to a libgcc named helper function) they obviously don't provide that one. (There has been discussions about whether they could start including these helpe functions somewhere, to help Clang cases, but it hasn't materialized yet at least.)

Overall, this is a Clang-cl toolchain issue - known for some time already; since this is a builtin type, the helpers for it should really be handled automatically. But since most use in MSVC style settings invoke the linker directly instead of via the compiler driver, the fix isn't entirely trivial. https://reviews.llvm.org/D134912#3828421 is the latest word on what would be a good way forward to handle it (from the toolchain point of view).

Also from a toolchain point of view, this is a bit tricky, since you normally can cross compile with clang-cl for any supported architecture as long as you've got MSVC and WinSDK, but when clang starts requiring the builtins, they you also need to have those built and available, for all architectures that you want to compile for. MSVC itself actually does ship with these files for some architectures (x86 and x64), but not for arm64 (or arm). So I think we should request that MSVC should start bundling clang_rt.*.lib for arm64 too, at least as many parts of compiler-rt as works on arm64 (IIRC the sanitizers don't support it yet), so that Clang could start automatically linking against it when needed (without needing to ship it along with Clang specifically for arm64 only).

In D138023#3927276, @omjavaid wrote:

How does flang work when built with MSVC proper, which doesn't have int128 as a built-in type at all?

We have not been using cl as they are available as emulated tools on WoA while clang-cl is native and faster. Although I tried cl just today but was not able to get the build working facing errors which are unrelated to this particular issue.

Ok, I see. (BTW since some months, MSVC should be available as properly native arm64 apps now too. But I don't expect you to try to set it up right now, I'm sure there's lots of things one would run into trying to build flang with it.)

I wonder why this isn't an issue with clang-cl builds on x86_64 too - the same issue should appear there too, if it uses int128 in the same way.

This issue appear when compiling flang project and it also appears on x86_64 when a combination of MSVC + Clang-cl is used. I have not tried a MSVC + cl build.

If it is possible to just decide not to use int128 for this build configuration, that's clearly the simplest solution. But if that's not possible, I guess something like this has to be done.

I guess Fortran extensively uses 128bit types and 128 bit division may not be avoidable in this case.

[omjavaid]

Corrects the diff error from last patch.

[omjavaid]

@mstorsjo Is it good to land?

[mstorsjo]

No objections from me on this patch, but I’m not really authoritative to approve things under flang I think.

[DavidTruby]

On MSVC flang uses a completely custom int128 implementation to get around it lacking int128 support, see: flang/include/flang/Common/uint128.h
I think with clang-cl on Windows we're better off using the builtins and using compiler-rt than using the custom implementation, so fixing this looks sensible.

LGTM

flang/CMakeLists.txt

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   set(FLANG_STANDALONE_BUILD ON)
 else()
   set(FLANG_STANDALONE_BUILD OFF)
 endif()
 
 # Must go below project(..)
 include(GNUInstallDirs)
 
+# MSVC + clang-cl build requires clang_rt.builtin.${target} library
+if (MSVC AND CMAKE_CXX_COMPILER_ID MATCHES Clang)
+  include(HandleCompilerRT)
+  find_compiler_rt_library(builtins CLANG_RT_BUILTINS_LIBRARY)
+  get_filename_component(LIBDIR "${CLANG_RT_BUILTINS_LIBRARY}" DIRECTORY)
+  if (IS_DIRECTORY "${LIBDIR}")
+    link_libraries(${CLANG_RT_BUILTINS_LIBRARY})
+  endif()
+endif()
+
 if (FLANG_STANDALONE_BUILD)
   set(FLANG_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
   if (NOT MSVC_IDE)
     set(LLVM_ENABLE_ASSERTIONS ${ENABLE_ASSERTIONS}
       CACHE BOOL "Enable assertions")
     # Assertions follow llvm's configuration.
     mark_as_advanced(LLVM_ENABLE_ASSERTIONS)
   endif()