From 775fe80796c717da4ed61e1cb7ace27268afc965 Mon Sep 17 00:00:00 2001
From: Sam Yates <yates@cscs.ch>
Date: Thu, 5 Jul 2018 16:36:34 +0200
Subject: [PATCH] Test for xlC and refuse to build with it. (#519)

Fixes issue #517.

Deprecate the IBM xlC compiler.
xlC generates code that is an order of a magnitude slower than gcc, while generating spurious warnings, and requiring hacks and workarounds to pass all tests.
Supporting it makes no sense.

* Add test and fatal error for xlC detection in CheckCompilerXLC.cmake.
* Move xlC 13 misdetection work around to CheckCompilerXLC.cmake.
* Remove xlC-specific compatibility workarounds from code.
---
 CMakeLists.txt                       |  4 +++
 arbor/algorithms.hpp                 |  8 ++++--
 arbor/math.hpp                       |  1 +
 arbor/util/index_into.hpp            | 10 ++++----
 arbor/util/iterutil.hpp              |  7 +++---
 arbor/util/meta.hpp                  | 17 ++++++++-----
 arbor/util/range.hpp                 |  1 +
 cmake/CheckCompilerXLC.cmake         | 17 +++++++++++++
 cmake/CompilerOptions.cmake          | 11 ---------
 include/arbor/schedule.hpp           |  2 +-
 include/arbor/util/compat.hpp        | 37 ++--------------------------
 include/arbor/util/uninitialized.hpp | 10 ++------
 test/unit/test_algorithms.cpp        |  4 +--
 test/unit/test_math.cpp              | 11 +++------
 14 files changed, 57 insertions(+), 83 deletions(-)
 create mode 100644 cmake/CheckCompilerXLC.cmake

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 54e0e212..7ba6d75d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -93,6 +93,10 @@ set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS "YES")
 
+# Detect and deprecate xlC.
+
+include("CheckCompilerXLC")
+
 # Compiler options common to library, examples, tests, etc.
 
 include("CompilerOptions")
diff --git a/arbor/algorithms.hpp b/arbor/algorithms.hpp
index b2596d70..7ae4a5c5 100644
--- a/arbor/algorithms.hpp
+++ b/arbor/algorithms.hpp
@@ -317,6 +317,10 @@ bool is_unique(const Seq& seq) {
 
 /// Binary search, because std::binary_search doesn't return information
 /// about where a match was found.
+
+// TODO: consolidate these with rangeutil routines; make them sentinel
+// friendly; use ADL for begin/end (simpler with C++14).
+
 template <typename It, typename T>
 It binary_find(It b, It e, const T& value) {
     auto it = std::lower_bound(b, e, value);
@@ -327,14 +331,14 @@ template <typename Seq, typename T>
 auto binary_find(const Seq& seq, const T& value)
     -> decltype(binary_find(std::begin(seq), std::end(seq), value))
 {
-    return binary_find(std::begin(seq), compat::end(seq), value);
+    return binary_find(std::begin(seq), std::end(seq), value);
 }
 
 template <typename Seq, typename T>
 auto binary_find(Seq& seq, const T& value)
     -> decltype(binary_find(std::begin(seq), std::end(seq), value))
 {
-    return binary_find(std::begin(seq), compat::end(seq), value);
+    return binary_find(std::begin(seq), std::end(seq), value);
 }
 
 } // namespace algorithms
diff --git a/arbor/math.hpp b/arbor/math.hpp
index f5a8cfc3..4db898af 100644
--- a/arbor/math.hpp
+++ b/arbor/math.hpp
@@ -8,6 +8,7 @@
 namespace arb {
 namespace math {
 
+// TODO: C++14 variable template
 template <typename T>
 T constexpr pi() {
     return T(3.1415926535897932384626433832795l);
diff --git a/arbor/util/index_into.hpp b/arbor/util/index_into.hpp
index cb31503d..879c3bac 100644
--- a/arbor/util/index_into.hpp
+++ b/arbor/util/index_into.hpp
@@ -17,10 +17,9 @@
 #include <type_traits>
 
 #include <arbor/assert.hpp>
-#include <arbor/util/compat.hpp>
 
-#include <util/meta.hpp>
-#include <util/range.hpp>
+#include "util/meta.hpp"
+#include "util/range.hpp"
 
 namespace arb {
 namespace util {
@@ -143,10 +142,11 @@ auto index_into(const Sub& sub, const Super& sup)
         >;
 
     using std::begin;
+    using std::end;
 
     auto canon = canonical_view(sub);
-    iterator b(begin(canon), compat::end(canon), begin(sup), compat::end(sup));
-    iterator e(compat::end(canon), compat::end(canon), begin(sup), compat::end(sup));
+    iterator b(canon.begin(), canon.end(), begin(sup), end(sup));
+    iterator e(canon.end(), canon.end(), begin(sup), end(sup));
 
     return range<iterator>(b, e);
 }
diff --git a/arbor/util/iterutil.hpp b/arbor/util/iterutil.hpp
index 0b37365d..53b68eec 100644
--- a/arbor/util/iterutil.hpp
+++ b/arbor/util/iterutil.hpp
@@ -10,8 +10,6 @@
 #include <type_traits>
 #include <utility>
 
-#include <arbor/util/compat.hpp>
-
 #include "util/meta.hpp"
 
 namespace arb {
@@ -72,15 +70,16 @@ distance(I first, E last) {
  * generic front() and back() methods for containers or ranges
  */
 
+// TODO: Use ADL begin and end when we avoid explicit return type in C++14
 template <typename Seq>
 auto front(Seq& seq) -> decltype(*std::begin(seq)) {
     return *std::begin(seq);
 }
 
+// TODO: Use ADL begin and end when we avoid explicit return type in C++14
 template <typename Seq>
 auto back(Seq& seq) -> decltype(*std::begin(seq)) {
-    // COMPAT: use own `end` implementation to work around xlC 13.1 bug.
-    return *upto(std::begin(seq), compat::end(seq));
+    return *upto(std::begin(seq), std::end(seq));
 }
 
 /*
diff --git a/arbor/util/meta.hpp b/arbor/util/meta.hpp
index da50ce2a..f1debc43 100644
--- a/arbor/util/meta.hpp
+++ b/arbor/util/meta.hpp
@@ -6,7 +6,6 @@
 #include <iterator>
 #include <type_traits>
 
-#include <arbor/util/compat.hpp>
 
 #include "util/deduce_return.hpp"
 
@@ -57,20 +56,24 @@ constexpr typename std::add_const<T>::type& as_const(T& t) {
     return t;
 }
 
-// Wrap cbegin in inner namespace in order to properly invoke ADL.
+// Wrap cbegin, cend in inner namespace in order to properly invoke ADL.
 
 namespace impl {
     using std::begin;
+    using std::end;
 
     template <typename T>
     constexpr auto cbegin_(const T& c) DEDUCED_RETURN_TYPE(begin(c))
+
+    template <typename T>
+    constexpr auto cend_(const T& c) DEDUCED_RETURN_TYPE(end(c))
 }
 
 template <typename T>
 constexpr auto cbegin(const T& c) DEDUCED_RETURN_TYPE(impl::cbegin_(c))
 
 template <typename T>
-constexpr auto cend(const T& c) DEDUCED_RETURN_TYPE(compat::end(c))
+constexpr auto cend(const T& c) DEDUCED_RETURN_TYPE(impl::cend_(c))
 
 // Use sequence `empty() const` method if exists, otherwise
 // compare begin and end.
@@ -87,10 +90,11 @@ namespace impl_empty {
     };
 
     using std::begin;
+    using std::end;
 
     template <typename Seq>
     constexpr bool empty(const Seq& seq, std::false_type) {
-        return begin(seq)==compat::end(seq);
+        return begin(seq)==end(seq);
     }
 
     template <typename Seq>
@@ -113,6 +117,7 @@ constexpr bool empty(const T (& c)[N]) noexcept {
 
 namespace impl_seqtrait {
     using std::begin;
+    using std::end;
 
     template <typename Seq, typename = void>
     struct data_returns_pointer: std::false_type {};
@@ -130,8 +135,8 @@ namespace impl_seqtrait {
         using difference_type = typename std::iterator_traits<iterator>::difference_type;
         using size_type = decltype(size(std::declval<Seq&>()));
         // For use with heterogeneous ranges:
-        using sentinel = decltype(compat::end(std::declval<Seq&>()));
-        using const_sentinel = decltype(compat::end(std::declval<const Seq&>()));
+        using sentinel = decltype(end(std::declval<Seq&>()));
+        using const_sentinel = decltype(end(std::declval<const Seq&>()));
 
         static constexpr bool is_contiguous = data_returns_pointer<Seq>::value;
     };
diff --git a/arbor/util/range.hpp b/arbor/util/range.hpp
index 078af71b..a47537bd 100644
--- a/arbor/util/range.hpp
+++ b/arbor/util/range.hpp
@@ -184,6 +184,7 @@ range<U, V> make_range(const std::pair<U, V>& iterators) {
 // Present a possibly sentinel-terminated range as an STL-compatible sequence
 // using the sentinel_iterator adaptor.
 
+// TODO: ADL begin/end with C++14 deduced return.
 template <typename Seq>
 auto canonical_view(Seq& s) ->
     range<sentinel_iterator_t<decltype(std::begin(s)), decltype(std::end(s))>>
diff --git a/cmake/CheckCompilerXLC.cmake b/cmake/CheckCompilerXLC.cmake
new file mode 100644
index 00000000..7354dd27
--- /dev/null
+++ b/cmake/CheckCompilerXLC.cmake
@@ -0,0 +1,17 @@
+# CMake (at least sometimes) misidentifies XL 13 for Linux as Clang.
+
+if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
+    try_compile(ignore ${CMAKE_BINARY_DIR} ${PROJECT_SOURCE_DIR}/cmake/dummy.cpp COMPILE_DEFINITIONS --version OUTPUT_VARIABLE cc_out)
+    string(REPLACE "\n" ";" cc_out "${cc_out}")
+    foreach(line ${cc_out})
+        if(line MATCHES "^IBM XL C")
+            set(CMAKE_CXX_COMPILER_ID "XL")
+        endif()
+    endforeach(line)
+endif()
+
+# If we _do_ find xlC, don't try and build: too many bugs!
+
+if(CMAKE_CXX_COMPILER_ID STREQUAL "XL")
+    message(FATAL_ERROR "Arbor does not support being built by the IBM xlC compiler")
+endif()
diff --git a/cmake/CompilerOptions.cmake b/cmake/CompilerOptions.cmake
index 84cba101..9ae81d6e 100644
--- a/cmake/CompilerOptions.cmake
+++ b/cmake/CompilerOptions.cmake
@@ -3,17 +3,6 @@
 set(CXXOPT_DEBUG "-g")
 set(CXXOPT_CXX11 "-std=c++11")
 
-# CMake (at least sometimes) misidentifies XL 13 for Linux as Clang.
-if(CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-    try_compile(ignore ${CMAKE_BINARY_DIR} ${PROJECT_SOURCE_DIR}/cmake/dummy.cpp COMPILE_DEFINITIONS --version OUTPUT_VARIABLE cc_out)
-    string(REPLACE "\n" ";" cc_out "${cc_out}")
-    foreach(line ${cc_out})
-        if(line MATCHES "^IBM XL C")
-            set(CMAKE_CXX_COMPILER_ID "XL")
-        endif()
-    endforeach(line)
-endif()
-
 if(CMAKE_CXX_COMPILER_ID MATCHES "XL")
     # CMake, bless its soul, likes to insert this unsupported flag. Hilarity ensues.
     string(REPLACE "-qhalt=e" "" CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS}")
diff --git a/include/arbor/schedule.hpp b/include/arbor/schedule.hpp
index 15ddadae..7ef42afc 100644
--- a/include/arbor/schedule.hpp
+++ b/include/arbor/schedule.hpp
@@ -109,7 +109,7 @@ public:
         start_index_(0)
     {
         using std::begin;
-        using compat::end; // TODO: replace with std::end when we nuke xlC support.
+        using std::end;
 
         times_.assign(begin(seq), end(seq));
         arb_assert(std::is_sorted(times_.begin(), times_.end()));
diff --git a/include/arbor/util/compat.hpp b/include/arbor/util/compat.hpp
index 0603e02c..026041ba 100644
--- a/include/arbor/util/compat.hpp
+++ b/include/arbor/util/compat.hpp
@@ -2,6 +2,8 @@
 
 /* Collection of compatibility workarounds to deal with compiler defects */
 
+// Note: workarounds for xlC removed; use of xlC to build Arbor is deprecated.
+
 #include <cstddef>
 #include <cmath>
 
@@ -25,34 +27,6 @@ constexpr bool using_gnu_compiler(int major=0, int minor=0, int patchlevel=0) {
 #endif
 }
 
-// std::end() broken with xlC 13.1.4; fixed in 13.1.5.
-
-namespace impl {
-    using std::end;
-    template <typename T>
-    auto end_(T& x) -> decltype(end(x)) { return end(x); }
-}
-
-template <typename T>
-auto end(T& x) -> decltype(impl::end_(x)) { return impl::end_(x); }
-
-template <typename T, std::size_t N>
-T* end(T (&x)[N]) { return &x[0]+N; }
-
-template <typename T, std::size_t N>
-const T* end(const T (&x)[N]) { return &x[0]+N; }
-
-// Work-around bad optimization reordering in xlC 13.1.4.
-// Note: still broken in xlC 14.1.0
-
-inline void compiler_barrier_if_xlc_leq(unsigned ver) {
-#if defined(__xlC__)
-    if (__xlC__<=ver) {
-        asm volatile ("" ::: "memory");
-    }
-#endif
-}
-
 // Work-around a bad inlining-related optimization with icpc 16.0.3 and -xMIC-AVX512,
 
 inline void compiler_barrier_if_icc_leq(unsigned ver) {
@@ -63,11 +37,4 @@ inline void compiler_barrier_if_icc_leq(unsigned ver) {
 #endif
 }
 
-// Work-around bad ordering of std::isinf() (sometimes) within switch, xlC 13.1.4;
-// wrapping the call within another function appears to be sufficient.
-// Note: still broken in xlC 14.1.0.
-
-template <typename X>
-inline constexpr bool isinf(X x) { return std::isinf(x); }
-
 } // namespace compat
diff --git a/include/arbor/util/uninitialized.hpp b/include/arbor/util/uninitialized.hpp
index 5c8e46e2..23455ff1 100644
--- a/include/arbor/util/uninitialized.hpp
+++ b/include/arbor/util/uninitialized.hpp
@@ -43,16 +43,10 @@ public:
     using const_rvalue_reference= const X&&;
 
     pointer ptr() {
-        // COMPAT: xlC 13.1.4 workaround (still broken in 14.1.0):
-        // should be equivalent to `return reinterpret_cast<X*>(&data)`.
-        compat::compiler_barrier_if_xlc_leq(0x0e01);
-        return static_cast<X*>(static_cast<void*>(&data));
+        return reinterpret_cast<X*>(&data);
     }
     const_pointer cptr() const {
-        // COMPAT: xlC 13.1.4 workaround (still broken in 14.1.0):
-        // should be equivalent to `return reinterpret_cast<const X*>(&data)`
-        compat::compiler_barrier_if_xlc_leq(0x0e01);
-        return static_cast<const X*>(static_cast<const void*>(&data));
+        return reinterpret_cast<const X*>(&data);
     }
 
     reference ref() { return *ptr(); }
diff --git a/test/unit/test_algorithms.cpp b/test/unit/test_algorithms.cpp
index d0fe5b01..7a0e8aa6 100644
--- a/test/unit/test_algorithms.cpp
+++ b/test/unit/test_algorithms.cpp
@@ -6,8 +6,6 @@
 
 #include "../gtest.h"
 
-#include <arbor/util/compat.hpp>
-
 #include "algorithms.hpp"
 #include "util/index_into.hpp"
 #include "util/meta.hpp"
@@ -538,7 +536,7 @@ template <typename Sub, typename Sup>
     }
 
     using std::begin;
-    using compat::end;
+    using std::end;
 
     auto sub_i = begin(sub);
     auto sup_i = begin(sup);
diff --git a/test/unit/test_math.cpp b/test/unit/test_math.cpp
index c7c9cd0b..93e2c457 100644
--- a/test/unit/test_math.cpp
+++ b/test/unit/test_math.cpp
@@ -3,8 +3,6 @@
 
 #include "../gtest.h"
 
-#include <arbor/util/compat.hpp>
-
 #include "math.hpp"
 
 using namespace arb::math;
@@ -85,20 +83,17 @@ TEST(math, infinity) {
     // check values for float, double, long double
     auto finf = infinity<float>();
     EXPECT_TRUE((std::is_same<float, decltype(finf)>::value));
-    // COMPAT: use compatibility wrapper for isinf() thanks to xlC 13.1 bug.
-    EXPECT_TRUE(compat::isinf(finf));
+    EXPECT_TRUE(std::isinf(finf));
     EXPECT_GT(finf, 0.f);
 
     auto dinf = infinity<double>();
     EXPECT_TRUE((std::is_same<double, decltype(dinf)>::value));
-    // COMPAT: use compatibility wrapper for isinf() thanks to xlC 13.1 bug.
-    EXPECT_TRUE(compat::isinf(dinf));
+    EXPECT_TRUE(std::isinf(dinf));
     EXPECT_GT(dinf, 0.0);
 
     auto ldinf = infinity<long double>();
     EXPECT_TRUE((std::is_same<long double, decltype(ldinf)>::value));
-    // COMPAT: use compatibility wrapper for isinf() thanks to xlC 13.1 bug.
-    EXPECT_TRUE(compat::isinf(ldinf));
+    EXPECT_TRUE(std::isinf(ldinf));
     EXPECT_GT(ldinf, 0.0l);
 
     // check default value promotes correctly (i.e., acts like INFINITY)
-- 
GitLab