[PATCH] D65493: Modernize atomic detection and usage

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added a comment.

This looks like a good simplification, but I think `call_once` could be simplified more.



================
Comment at: llvm/cmake/modules/CheckAtomic.cmake:46
+      if (NOT HAVE_ATOMICS_WITH_LIB)
+	message(FATAL_ERROR "Host compiler must support atomic!")
       endif()
----------------
Indentation?


================
Comment at: llvm/cmake/modules/CheckAtomic.cmake:75
 
-## TODO: This define is only used for the legacy atomic operations in
-## llvm's Atomic.h, which should be replaced.  Other code simply
-## assumes C++11 <atomic> works.
-CHECK_CXX_SOURCE_COMPILES("
-#ifdef _MSC_VER
-#include <windows.h>
-#endif
-int main() {
-#ifdef _MSC_VER
-        volatile LONG val = 1;
-        MemoryBarrier();
-        InterlockedCompareExchange(&val, 0, 1);
-        InterlockedIncrement(&val);
-        InterlockedDecrement(&val);
-#else
-        volatile unsigned long val = 1;
-        __sync_synchronize();
-        __sync_val_compare_and_swap(&val, 1, 0);
-        __sync_add_and_fetch(&val, 1);
-        __sync_sub_and_fetch(&val, 1);
-#endif
-        return 0;
-      }
-" LLVM_HAS_ATOMICS)
-
 if( NOT LLVM_HAS_ATOMICS )
   message(STATUS "Warning: LLVM will be built thread-unsafe because atomic builtins are missing")
----------------
Does anything else set `LLVM_HAS_ATOMICS`?


================
Comment at: llvm/include/llvm/Support/Threading.h:111
+      std::atomic_thread_fence(std::memory_order_seq_cst);
       TsanIgnoreWritesBegin();
       TsanHappensBefore(&flag.status);
----------------
Not sure we still need the tsan hooks when using `std::atomic`.


================
Comment at: llvm/include/llvm/Support/Threading.h:118
+      InitStatus tmp = flag.status;
+      std::atomic_thread_fence(std::memory_order_seq_cst);
       while (tmp != Done) {
----------------
Do we actually need thread fences here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65493/new/

https://reviews.llvm.org/D65493