Remove default CPU_TARGET=avx (facebookincubator#10346)

Summary: The default should be empty so that the CPU_TARGET/CPU_ARCH will be inferred. Remove SSE support for macos as developers are mostly on Intel avx or Apple arm. Pull Request resolved: facebookincubator#10346 Reviewed By: bikramSingh91 Differential Revision: D59592699 Pulled By: pedroerp fbshipit-source-id: ca049ca88e99a1ee7ce33857e5117e39093c360b
PHILO-HE · Jul 10, 2024 · 7f2d7ad · 7f2d7ad
1 parent 319bc2c
commit 7f2d7ad
Show file tree

Hide file tree

Showing 10 changed files with 43 additions and 74 deletions.
diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml
@@ -47,16 +47,14 @@ jobs:
         include:
           - name: Check
             file: "scripts/check-container.dockfile"
-            args: "cpu_target=avx"
-            tags: "ghcr.io/facebookincubator/velox-dev:check-avx"
+            tags: "ghcr.io/facebookincubator/velox-dev:check"
           - name: Centos 9
             file: "scripts/centos.dockerfile"
-            args: "cpu_target=avx"
             tags: "ghcr.io/facebookincubator/velox-dev:centos9"
           - name: Dev
             file: "scripts/ubuntu-22.04-cpp.dockerfile"
             args: ""
-            tags: "ghcr.io/facebookincubator/velox-dev:amd64-ubuntu-22.04-avx"
+            tags: "ghcr.io/facebookincubator/velox-dev:ubuntu-22.04"
 
     steps:
       - name: Login to GitHub Container Registry
@@ -90,7 +88,6 @@ jobs:
         include:
           - name: Adapters
             file: "scripts/adapters.dockerfile"
-            args: "cpu_target=avx"
             tags: "ghcr.io/facebookincubator/velox-dev:adapters"
           - name: Presto Java
             file: "scripts/prestojava-container.dockerfile"

diff --git a/README.md b/README.md
@@ -104,8 +104,6 @@ $ export PATH=/opt/homebrew/opt/m4/bin:$PATH
 $ M4=/usr/bin/gm4 make
 ```
 
-You can also produce intel binaries on an M1, use `CPU_TARGET="sse"` for the above.
-
 ### Setting up on Ubuntu (20.04 or later)
 
 The supported architectures are x86_64 (avx, sse), and AArch64 (apple-m1+crc, neoverse-n1).

diff --git a/docker-compose.yml b/docker-compose.yml
@@ -21,7 +21,7 @@ services:
     #   or
     #   docker-compose run -e NUM_THREADS=<NUMBER_OF_THREADS_TO_USE> --rm ubuntu-cpp
     #   to set the number of threads used during compilation
-    image: ghcr.io/facebookincubator/velox-dev:amd64-ubuntu-22.04-avx
+    image: ghcr.io/facebookincubator/velox-dev:ubuntu-22.04
     build:
       context: .
       dockerfile: scripts/ubuntu-22.04-cpp.dockerfile

diff --git a/scripts/adapters.dockerfile b/scripts/adapters.dockerfile
@@ -14,8 +14,6 @@
 # Build the test and build container for presto_cpp
 ARG image=ghcr.io/facebookincubator/velox-dev:centos9
 FROM $image
-ARG cpu_target=avx
-ENV CPU_TARGET=$cpu_target
 
 COPY scripts/setup-adapters.sh /
 RUN mkdir build && ( cd build &&  source /opt/rh/gcc-toolset-12/enable && \

diff --git a/scripts/centos.dockerfile b/scripts/centos.dockerfile
@@ -14,8 +14,6 @@
 # Build the test and build container for presto_cpp
 ARG image=quay.io/centos/centos:stream9
 FROM $image
-ARG cpu_target=avx
-ENV CPU_TARGET=$cpu_target
 
 COPY scripts/setup-helper-functions.sh /
 COPY scripts/setup-centos9.sh /
@@ -27,7 +25,6 @@ RUN mkdir build && ( cd build && bash /setup-centos9.sh ) && rm -rf build && \
         dnf install -y -q gh jq && \
         dnf clean all
 
-
 ENV CC=/opt/rh/gcc-toolset-12/root/bin/gcc \
     CXX=/opt/rh/gcc-toolset-12/root/bin/g++
 

diff --git a/scripts/check-container.dockfile b/scripts/check-container.dockfile
@@ -12,7 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 FROM amd64/ubuntu:24.04
-ARG cpu_target
 COPY scripts/setup-check.sh /root
 COPY scripts/setup-helper-functions.sh /
-RUN CPU_TARGET="$cpu_target" bash /root/setup-check.sh
+RUN bash /root/setup-check.sh
diff --git a/scripts/setup-centos9.sh b/scripts/setup-centos9.sh
@@ -30,9 +30,8 @@ set -efx -o pipefail
 # so that some low level types are the same size. Also, disable warnings.
 SCRIPTDIR=$(dirname "${BASH_SOURCE[0]}")
 source $SCRIPTDIR/setup-helper-functions.sh
-CPU_TARGET="${CPU_TARGET:-avx}"
 NPROC=$(getconf _NPROCESSORS_ONLN)
-export CFLAGS=$(get_cxx_flags $CPU_TARGET)  # Used by LZO.
+export CFLAGS=$(get_cxx_flags)  # Used by LZO.
 export CXXFLAGS=$CFLAGS  # Used by boost.
 export CPPFLAGS=$CFLAGS  # Used by LZO.
 CMAKE_BUILD_TYPE="${BUILD_TYPE:-Release}"

diff --git a/scripts/setup-helper-functions.sh b/scripts/setup-helper-functions.sh
@@ -59,7 +59,7 @@ function github_checkout {
 }
 
 # get_cxx_flags [$CPU_ARCH]
-# Sets and exports the variable VELOX_CXX_FLAGS with appropriate compiler flags.
+# Echos appropriate compiler flags.
 # If $CPU_ARCH is set then we use that else we determine best possible set of flags
 # to use based on current cpu architecture.
 # The goal of this function is to consolidate all architecture specific flags to one
@@ -74,65 +74,49 @@ function github_checkout {
 # CXX_FLAGS=$(get_cxx_flags "avx")
 
 function get_cxx_flags {
-  local CPU_ARCH=$1
-
-  local OS
-  OS=$(uname)
-  local MACHINE
-  MACHINE=$(uname -m)
-  ADDITIONAL_FLAGS=""
-
-  if [[ -z "$CPU_ARCH" ]] || [[ $CPU_ARCH == "unknown" ]]; then
-    if [ "$OS" = "Darwin" ]; then
-
-      if [ "$MACHINE" = "x86_64" ]; then
-        local CPU_CAPABILITIES
-        CPU_CAPABILITIES=$(sysctl -a | grep machdep.cpu.features | awk '{print tolower($0)}')
-
-        if [[ $CPU_CAPABILITIES =~ "avx" ]]; then
-          CPU_ARCH="avx"
-        else
-          CPU_ARCH="sse"
-        fi
-
-      elif [[ $(sysctl -a | grep machdep.cpu.brand_string) =~ "Apple" ]]; then
-        # Apple silicon.
-        CPU_ARCH="arm64"
-      fi
-
-    # On MacOs prevent the flood of translation visibility settings warnings.
-    ADDITIONAL_FLAGS="-fvisibility=hidden -fvisibility-inlines-hidden"
-    else [ "$OS" = "Linux" ];
-
-      local CPU_CAPABILITIES
-      CPU_CAPABILITIES=$(cat /proc/cpuinfo | grep flags | head -n 1| awk '{print tolower($0)}')
-
-      if [[ "$CPU_CAPABILITIES" =~ "avx" ]]; then
-            CPU_ARCH="avx"
-      elif [[ "$CPU_CAPABILITIES" =~ "sse" ]]; then
-            CPU_ARCH="sse"
-      elif [ "$MACHINE" = "aarch64" ]; then
-            CPU_ARCH="aarch64"
-      fi
-    fi
+  local CPU_ARCH=${1:-""}
+  local OS=$(uname)
+  local MACHINE=$(uname -m)
+
+  if [[ -z "$CPU_ARCH" ]]; then
+   if [ "$OS" = "Darwin" ]; then
+     if [ "$MACHINE" = "arm64" ]; then
+       CPU_ARCH="arm64"
+     else
+       CPU_ARCH="avx"
+     fi
+   elif [ "$OS" = "Linux" ]; then
+     if [ "$MACHINE" = "aarch64" ]; then
+           CPU_ARCH="aarch64"
+     else
+       local CPU_CAPABILITIES=$(cat /proc/cpuinfo | grep flags | head -n 1| awk '{print tolower($0)}')
+       # Even though the default is avx, we need this check since avx machines support sse as well.
+       if [[ $CPU_CAPABILITIES =~ "avx" ]]; then
+           CPU_ARCH="avx"
+       elif [[ $CPU_CAPABILITIES =~ "sse" ]]; then
+           CPU_ARCH="sse"
+       fi
+     fi
+   else
+     echo "Unsupported platform $OS"; exit 1;
+   fi
   fi
-
   case $CPU_ARCH in
 
     "arm64")
-      echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden $ADDITIONAL_FLAGS"
+      echo -n "-mcpu=apple-m1+crc -std=c++17 -fvisibility=hidden"
     ;;
 
     "avx")
-      echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2 $ADDITIONAL_FLAGS"
+      echo -n "-mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2"
     ;;
 
     "sse")
-      echo -n "-msse4.2 -std=c++17 $ADDITIONAL_FLAGS"
+      echo -n "-msse4.2 -std=c++17"
     ;;
 
     "aarch64")
-      echo -n "-mcpu=neoverse-n1 -std=c++17 $ADDITIONAL_FLAGS"
+      echo -n "-mcpu=neoverse-n1 -std=c++17"
     ;;
   *)
     echo -n "Architecture not supported!"
@@ -158,8 +142,7 @@ function cmake_install {
     ${SUDO} rm -rf "${BINARY_DIR}"
   fi
   mkdir -p "${BINARY_DIR}"
-  CPU_TARGET="${CPU_TARGET:-unknown}"
-  COMPILER_FLAGS=$(get_cxx_flags $CPU_TARGET)
+  COMPILER_FLAGS=$(get_cxx_flags)
 
   # CMAKE_POSITION_INDEPENDENT_CODE is required so that Velox can be built into dynamic libraries \
   cmake -Wno-dev -B"${BINARY_DIR}" \
@@ -171,8 +154,8 @@ function cmake_install {
     -DCMAKE_CXX_FLAGS="$COMPILER_FLAGS" \
     -DBUILD_TESTING=OFF \
     "$@"
-
-  cmake --build "${BINARY_DIR}"
+  # Exit if the build fails.
+  cmake --build "${BINARY_DIR}" || { echo 'build failed' ; exit 1; }
   ${SUDO} cmake --install "${BINARY_DIR}"
 }
 
diff --git a/scripts/setup-ubuntu.sh b/scripts/setup-ubuntu.sh
@@ -32,8 +32,7 @@ source $SCRIPTDIR/setup-helper-functions.sh
 
 # Folly must be built with the same compiler flags so that some low level types
 # are the same size.
-CPU_TARGET="${CPU_TARGET:-avx}"
-COMPILER_FLAGS=$(get_cxx_flags "$CPU_TARGET")
+COMPILER_FLAGS=$(get_cxx_flags)
 export COMPILER_FLAGS
 FB_OS_VERSION=v2024.05.20.00
 FMT_VERSION=10.1.1

diff --git a/scripts/ubuntu-22.04-cpp.dockerfile b/scripts/ubuntu-22.04-cpp.dockerfile
@@ -11,10 +11,7 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-ARG base=amd64/ubuntu:22.04
-# Set a default timezone, can be overriden via ARG
-ARG tz="Europe/Madrid"
-
+ARG base=ubuntu:22.04
 FROM ${base}
 
 SHELL ["/bin/bash", "-o", "pipefail", "-c"]
@@ -32,6 +29,8 @@ ADD scripts /velox/scripts/
 # are required to avoid tzdata installation
 # to prompt for region selection.
 ARG DEBIAN_FRONTEND="noninteractive"
+# Set a default timezone, can be overriden via ARG
+ARG tz="Etc/UTC"
 ENV TZ=${tz}
 RUN /velox/scripts/setup-ubuntu.sh