From 7f2d7adaa1544c72129fd1c8d0766755ff354455 Mon Sep 17 00:00:00 2001 From: Deepak Majeti Date: Wed, 10 Jul 2024 11:30:02 -0700 Subject: [PATCH] Remove default CPU_TARGET=avx (#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: https://github.com/facebookincubator/velox/pull/10346 Reviewed By: bikramSingh91 Differential Revision: D59592699 Pulled By: pedroerp fbshipit-source-id: ca049ca88e99a1ee7ce33857e5117e39093c360b --- .github/workflows/docker.yml | 7 +-- README.md | 2 - docker-compose.yml | 2 +- scripts/adapters.dockerfile | 2 - scripts/centos.dockerfile | 3 - scripts/check-container.dockfile | 3 +- scripts/setup-centos9.sh | 3 +- scripts/setup-helper-functions.sh | 85 ++++++++++++----------------- scripts/setup-ubuntu.sh | 3 +- scripts/ubuntu-22.04-cpp.dockerfile | 7 +-- 10 files changed, 43 insertions(+), 74 deletions(-) mode change 100644 => 100755 scripts/setup-helper-functions.sh diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 3a8397d1aca9..932fd41d1339 100644 --- 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 index 4d7ae853d758..1618fccf39fa 100644 --- 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 index 290f52760d70..6904baa543ee 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -21,7 +21,7 @@ services: # or # docker-compose run -e NUM_THREADS= --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 index 73864ced11db..7dac26b94992 100644 --- 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 index 36e2efad6c2e..50a18e9a7243 100644 --- 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 index 32a1704f1b22..9240a97dcd8c 100644 --- 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 index 487dadba8af9..47ee0c08fdbe 100755 --- 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 old mode 100644 new mode 100755 index 7df6a9464ac6..73a52a54555f --- 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 index e765958038bb..d0f02ecf200b 100755 --- 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 index 2aa1ee7cbb3f..ea98a0f64b9e 100644 --- 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