Re: [FFmpeg-devel] [PATCH 1/2] lavu/bswap: remove some inline assembler

From: Sean McGovern <gseanmcg@gmail.com>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/2] lavu/bswap: remove some inline assembler
Date: Fri, 7 Jun 2024 17:17:59 -0400
Message-ID: <CAPBf_OkaSpkkgbDXV4NAxk4s7HGb=-KS4Z-sBkdnyBS81VJ4zw@mail.gmail.com> (raw)
In-Reply-To: <20240607181947.330026-1-remi@remlab.net>

On Fri, Jun 7, 2024 at 2:20 PM Rémi Denis-Courmont <remi@remlab.net> wrote:
>
> C code or compiler built-ins are preferable over inline assembler for
> byte-swaps as it allows for better optimisations (e.g. instruction
> scheduling) which would otherwise be impossible.
>
> As with f64c2e710fa1a7b59753224e717f57c48462076f for x86 and Arm,
> this removes the inline assembler on GCC (and Clang) since we now
> require recent enough compiler versions (this indeed seems to work on
> AArch64).
> ---
>  libavutil/aarch64/bswap.h | 56 ---------------------------------------
>  libavutil/avr32/bswap.h   | 44 ------------------------------
>  libavutil/bswap.h         |  8 +-----
>  libavutil/sh4/bswap.h     | 48 ---------------------------------
>  4 files changed, 1 insertion(+), 155 deletions(-)
>  delete mode 100644 libavutil/aarch64/bswap.h
>  delete mode 100644 libavutil/avr32/bswap.h
>  delete mode 100644 libavutil/sh4/bswap.h
>
> diff --git a/libavutil/aarch64/bswap.h b/libavutil/aarch64/bswap.h
> deleted file mode 100644
> index 7abca657ba..0000000000
> --- a/libavutil/aarch64/bswap.h
> +++ /dev/null
> @@ -1,56 +0,0 @@
> -/*
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -#ifndef AVUTIL_AARCH64_BSWAP_H
> -#define AVUTIL_AARCH64_BSWAP_H
> -
> -#include <stdint.h>
> -#include "config.h"
> -#include "libavutil/attributes.h"
> -
> -#if HAVE_INLINE_ASM
> -
> -#define av_bswap16 av_bswap16
> -static av_always_inline av_const unsigned av_bswap16(unsigned x)
> -{
> -    unsigned y;
> -
> -    __asm__("rev16 %w0, %w1" : "=r"(y) : "r"(x));
> -    return y;
> -}
> -
> -#define av_bswap32 av_bswap32
> -static av_always_inline av_const uint32_t av_bswap32(uint32_t x)
> -{
> -    uint32_t y;
> -
> -    __asm__("rev %w0, %w1" : "=r"(y) : "r"(x));
> -    return y;
> -}
> -
> -#define av_bswap64 av_bswap64
> -static av_always_inline av_const uint64_t av_bswap64(uint64_t x)
> -{
> -    uint64_t y;
> -
> -    __asm__("rev %0, %1" : "=r"(y) : "r"(x));
> -    return y;
> -}
> -
> -#endif /* HAVE_INLINE_ASM */
> -#endif /* AVUTIL_AARCH64_BSWAP_H */
> diff --git a/libavutil/avr32/bswap.h b/libavutil/avr32/bswap.h
> deleted file mode 100644
> index e79d53f369..0000000000
> --- a/libavutil/avr32/bswap.h
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -/*
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -#ifndef AVUTIL_AVR32_BSWAP_H
> -#define AVUTIL_AVR32_BSWAP_H
> -
> -#include <stdint.h>
> -#include "config.h"
> -#include "libavutil/attributes.h"
> -
> -#if HAVE_INLINE_ASM
> -
> -#define av_bswap16 av_bswap16
> -static av_always_inline av_const uint16_t av_bswap16(uint16_t x)
> -{
> -    __asm__ ("swap.bh %0" : "+r"(x));
> -    return x;
> -}
> -
> -#define av_bswap32 av_bswap32
> -static av_always_inline av_const uint32_t av_bswap32(uint32_t x)
> -{
> -    __asm__ ("swap.b  %0" : "+r"(x));
> -    return x;
> -}
> -
> -#endif /* HAVE_INLINE_ASM */
> -
> -#endif /* AVUTIL_AVR32_BSWAP_H */
> diff --git a/libavutil/bswap.h b/libavutil/bswap.h
> index 1528906f93..b339c90b9b 100644
> --- a/libavutil/bswap.h
> +++ b/libavutil/bswap.h
> @@ -34,16 +34,10 @@
>
>  #include "config.h"
>
> -#if   ARCH_AARCH64
> -#   include "aarch64/bswap.h"
> -#elif ARCH_ARM
> +#if ARCH_ARM
>  #   include "arm/bswap.h"
> -#elif ARCH_AVR32
> -#   include "avr32/bswap.h"
>  #elif ARCH_RISCV
>  #   include "riscv/bswap.h"
> -#elif ARCH_SH4
> -#   include "sh4/bswap.h"
>  #elif ARCH_X86
>  #   include "x86/bswap.h"
>  #endif
> diff --git a/libavutil/sh4/bswap.h b/libavutil/sh4/bswap.h
> deleted file mode 100644
> index 48dd27f806..0000000000
> --- a/libavutil/sh4/bswap.h
> +++ /dev/null
> @@ -1,48 +0,0 @@
> -/*
> - * This file is part of FFmpeg.
> - *
> - * FFmpeg is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2.1 of the License, or (at your option) any later version.
> - *
> - * FFmpeg is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with FFmpeg; if not, write to the Free Software
> - * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> - */
> -
> -/**
> - * @file
> - * byte swapping routines
> - */
> -
> -#ifndef AVUTIL_SH4_BSWAP_H
> -#define AVUTIL_SH4_BSWAP_H
> -
> -#include <stdint.h>
> -#include "config.h"
> -#include "libavutil/attributes.h"
> -
> -#define av_bswap16 av_bswap16
> -static av_always_inline av_const uint16_t av_bswap16(uint16_t x)
> -{
> -    __asm__("swap.b %0,%0" : "+r"(x));
> -    return x;
> -}
> -
> -#define av_bswap32 av_bswap32
> -static av_always_inline av_const uint32_t av_bswap32(uint32_t x)
> -{
> -    __asm__("swap.b %0,%0\n"
> -            "swap.w %0,%0\n"
> -            "swap.b %0,%0\n"
> -            : "+r"(x));
> -    return x;
> -}
> -
> -#endif /* AVUTIL_SH4_BSWAP_H */
> --
> 2.45.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

This also looks good to me.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".