[OpenMP] Fix clang-cl build error regarding TSX intrinsics

[jlpeyton]

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=49339

The CMake check for the RTM intrinsics needs the -mrtm flag to be set during the test. This way clang-cl correctly detects it has the _xbegin() intrinsic. Otherwise, the CMake check fails.

[hans]

I tried this, and it didn't fix the build error for me.

In the CMake log I see this:

-- Performing Test LIBOMP_HAVE_MRTM_FLAG
-- Performing Test LIBOMP_HAVE_MRTM_FLAG - Success
[..]
-- Performing Test LIBOMP_HAVE_RTM_INTRINSICS
-- Performing Test LIBOMP_HAVE_RTM_INTRINSICS - Failed

And in CMakeError.log:

Performing C++ SOURCE FILE Test LIBOMP_HAVE_RTM_INTRINSICS failed with the following output:
Change Dir: C:/src/llvm.monorepo/build.32selfhost/CMakeFiles/CMakeTmp

Run Build Command(s):C:/src/depot_tools/ninja.exe cmTC_fb778 && [1/2] Building CXX object CMakeFiles\cmTC_fb778.dir\src.cxx.obj
FAILED: CMakeFiles/cmTC_fb778.dir/src.cxx.obj 
c:\src\llvm.monorepo\build.32\bin\clang-cl.exe  /nologo -TP -DATTRIBUTE_RTM -DATTRIBUTE_WAITPKG -DIMMINTRIN_H  /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /bigobj /W4  -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion /Gw -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-extra -Wno-pedantic -DLIBOMP_HAVE_RTM_INTRINSICS  -Werror=unguarded-availability-new /MTd /Zi /Ob0 /Od /RTC1   -std:c++14 /showIncludes /FoCMakeFiles\cmTC_fb778.dir\src.cxx.obj /FdCMakeFiles\cmTC_fb778.dir\ -c src.cxx
src.cxx(12,16): error: use of undeclared identifier '_xbegin'
        return _xbegin();
               ^
1 error generated.
ninja: build stopped: subcommand failed.


Source file was:
// check for attribute rtm and rtm intrinsics
      #ifdef IMMINTRIN_H
      #include <immintrin.h>
      #endif
      #ifdef INTRIN_H
      #include <intrin.h>
      #endif
      #ifdef ATTRIBUTE_RTM
      __attribute__((target("rtm")))
      #endif
      static inline int __kmp_xbegin() {
        return _xbegin();
      }
      int main() { int a = __kmp_xbegin(); return a; }

It seems the -mrtm flag was never passed to clang-cl.

But if I change the patch to do

set(CMAKE_REQUIRED_FLAGS)
libomp_append(CMAKE_REQUIRED_FLAGS -mrtm LIBOMP_HAVE_MRTM_FLAG)

It works.

I'm not familiar with libomp's cmake stuff, but maybe libomp_append doesn't work if the variable is previously not defined?
My tweak should maybe not be taken as-is, since it would overwrite CMAKE_REQUIRED_FLAGS if it was non-empty.

[hans]

Oh, and I just realized: just because my machine supports RTM doesn't necessarily mean we want to use it, since these builds will then be used on all kinds of machines. This might be a bigger problem with the runtime's build config though.

[jlpeyton]

Since CMake's documentation states that the CMAKE_REQURIED_FLAGS variable is a string rather than a list, use simple set() to assign it.

[jlpeyton]

In D97413#2587137, @hans wrote:

Oh, and I just realized: just because my machine supports RTM doesn't necessarily mean we want to use it, since these builds will then be used on all kinds of machines. This might be a bigger problem with the runtime's build config though.

I think only the build compiler needs to support the instructions, not your machine.
This shouldn't cause a problem since the RTM instrinsics are only used inside a specific lock kind which the user must select using an environment variable.

[hans]

In D97413#2588041, @jlpeyton wrote:

In D97413#2587137, @hans wrote:

Oh, and I just realized: just because my machine supports RTM doesn't necessarily mean we want to use it, since these builds will then be used on all kinds of machines. This might be a bigger problem with the runtime's build config though.

I think only the build compiler needs to support the instructions, not your machine.
This shouldn't cause a problem since the RTM instrinsics are only used inside a specific lock kind which the user must select using an environment variable.

I see. Thanks for clarifying!

openmp/runtime/cmake/config-ix.cmake

179

I looked for how CMAKE_REQUIRED_FLAGS is used in other places, and this pattern seems common:

set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -std=c++11")

Maybe that is safer, in case CMAKE_REQUIRED_FLAGS already holds some flag that is important for the build.

[jlpeyton]

Ok, I see what you're saying. I've copied the exact idiom used in the LLVM CMake where CMAKE_REQUIRED_FLAGS is temporarily stored in OLD_CMAKE_REQUIRED_FLAGS and then restored after the check.

[jlpeyton]

Ok, this actually restores using OLD_CMAKE_REQUIRED_FLAGS

[hans]

Looks good to me. Thanks!

openmp/runtime/cmake/config-ix.cmake

set(source_code "// check for attribute and wait pkg intrinsics
       #ifdef ATTRIBUTE_WAITPKG
       __attribute__((target(\"waitpkg\")))
       #endif
       static inline int __kmp_umwait(unsigned hint, unsigned long long counter) {
         return _umwait(hint, counter);
       }
       int main() { int a = __kmp_umwait(0, 1000); return a; }")
   check_cxx_source_compiles("${source_code}" LIBOMP_HAVE_WAITPKG_INTRINSICS)
+  set(OLD_CMAKE_REQUIRED_FLAGS ${CMAKE_REQUIRED_FLAGS})
+  if (LIBOMP_HAVE_MRTM_FLAG)
+    set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -mrtm")
+  endif()
   set(source_code "// check for attribute rtm and rtm intrinsics
       #ifdef IMMINTRIN_H
       #include <immintrin.h>
       #endif
       #ifdef INTRIN_H
       #include <intrin.h>
       #endif
       #ifdef ATTRIBUTE_RTM
       __attribute__((target(\"rtm\")))
       #endif
       static inline int __kmp_xbegin() {
         return _xbegin();
       }
       int main() { int a = __kmp_xbegin(); return a; }")
   check_cxx_source_compiles("${source_code}" LIBOMP_HAVE_RTM_INTRINSICS)
   set(CMAKE_REQUIRED_DEFINITIONS)
+  set(CMAKE_REQUIRED_FLAGS ${OLD_CMAKE_REQUIRED_FLAGS})
 endif()
 
 # Find perl executable
 # Perl is used to create omp.h (and other headers) along with kmp_i18n_id.inc and kmp_i18n_default.inc
 find_package(Perl REQUIRED)
 # The perl scripts take the --os=/--arch= flags which expect a certain format for operating systems and arch's.
 # Until the perl scripts are removed, the most portable way to handle this is to have all operating systems that
 # are neither Windows nor Mac (Most Unix flavors) be considered lin to the perl scripts.  This is rooted