| Message ID | 20240607164921.19435-1-remi@remlab.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavu/arm: remove GCC 4.6- stuff | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Fri, Jun 7, 2024 at 12:49 PM Rémi Denis-Courmont <remi@remlab.net> wrote: > > Since the C11 support is required, those GCC versions can no longer be > supported anyhow. > --- > libavutil/arm/bswap.h | 20 -------- > libavutil/arm/intreadwrite.h | 91 ------------------------------------ > libavutil/intreadwrite.h | 4 +- > 3 files changed, 1 insertion(+), 114 deletions(-) > delete mode 100644 libavutil/arm/intreadwrite.h > > diff --git a/libavutil/arm/bswap.h b/libavutil/arm/bswap.h > index c3460e035d..48fefa4da3 100644 > --- a/libavutil/arm/bswap.h > +++ b/libavutil/arm/bswap.h > @@ -45,26 +45,6 @@ static av_always_inline av_const unsigned av_bswap16(unsigned x) > return y; > } > #endif > - > -#if AV_GCC_VERSION_AT_MOST(4,4) > -#define av_bswap32 av_bswap32 > -static av_always_inline av_const uint32_t av_bswap32(uint32_t x) > -{ > - uint32_t y; > -#if HAVE_ARMV6_INLINE > - __asm__("rev %0, %1" : "=r"(y) : "r"(x)); > -#else > - uint32_t t; > - __asm__ ("eor %1, %2, %2, ror #16 \n\t" > - "bic %1, %1, #0xFF0000 \n\t" > - "mov %0, %2, ror #8 \n\t" > - "eor %0, %0, %1, lsr #8 \n\t" > - : "=r"(y), "=&r"(t) : "r"(x)); > -#endif /* HAVE_ARMV6_INLINE */ > - return y; > -} > -#endif /* AV_GCC_VERSION_AT_MOST(4,4) */ > - > #endif /* __ARMCC_VERSION */ > > #endif /* AVUTIL_ARM_BSWAP_H */ > diff --git a/libavutil/arm/intreadwrite.h b/libavutil/arm/intreadwrite.h > deleted file mode 100644 > index 60fc860cbb..0000000000 > --- a/libavutil/arm/intreadwrite.h > +++ /dev/null > @@ -1,91 +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_ARM_INTREADWRITE_H > -#define AVUTIL_ARM_INTREADWRITE_H > - > -#include <stdint.h> > -#include "config.h" > -#include "libavutil/attributes.h" > - > -#if HAVE_FAST_UNALIGNED && HAVE_INLINE_ASM && AV_GCC_VERSION_AT_MOST(4,6) > - > -#define AV_RN16 AV_RN16 > -static av_always_inline unsigned AV_RN16(const void *p) > -{ > - const uint8_t *q = p; > - unsigned v; > -#if AV_GCC_VERSION_AT_MOST(4,5) > - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(*(const uint16_t *)q)); > -#elif defined __thumb__ > - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(q[0]), "m"(q[1])); > -#else > - __asm__ ("ldrh %0, %1" : "=r"(v) : "Uq"(q[0]), "m"(q[1])); > -#endif > - return v; > -} > - > -#define AV_WN16 AV_WN16 > -static av_always_inline void AV_WN16(void *p, uint16_t v) > -{ > - __asm__ ("strh %1, %0" : "=m"(*(uint16_t *)p) : "r"(v)); > -} > - > -#define AV_RN32 AV_RN32 > -static av_always_inline uint32_t AV_RN32(const void *p) > -{ > - const struct __attribute__((packed)) { uint32_t v; } *q = p; > - uint32_t v; > - __asm__ ("ldr %0, %1" : "=r"(v) : "m"(*q)); > - return v; > -} > - > -#define AV_WN32 AV_WN32 > -static av_always_inline void AV_WN32(void *p, uint32_t v) > -{ > - __asm__ ("str %1, %0" : "=m"(*(uint32_t *)p) : "r"(v)); > -} > - > -#if HAVE_ASM_MOD_Q > - > -#define AV_RN64 AV_RN64 > -static av_always_inline uint64_t AV_RN64(const void *p) > -{ > - const struct __attribute__((packed)) { uint32_t v; } *q = p; > - uint64_t v; > - __asm__ ("ldr %Q0, %1 \n\t" > - "ldr %R0, %2 \n\t" > - : "=&r"(v) > - : "m"(q[0]), "m"(q[1])); > - return v; > -} > - > -#define AV_WN64 AV_WN64 > -static av_always_inline void AV_WN64(void *p, uint64_t v) > -{ > - __asm__ ("str %Q2, %0 \n\t" > - "str %R2, %1 \n\t" > - : "=m"(*(uint32_t*)p), "=m"(*((uint32_t*)p+1)) > - : "r"(v)); > -} > - > -#endif /* HAVE_ASM_MOD_Q */ > - > -#endif /* HAVE_INLINE_ASM */ > - > -#endif /* AVUTIL_ARM_INTREADWRITE_H */ > diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h > index d0a5773b54..ccdbf79b74 100644 > --- a/libavutil/intreadwrite.h > +++ b/libavutil/intreadwrite.h > @@ -64,9 +64,7 @@ typedef union { > > #include "config.h" > > -#if ARCH_ARM > -# include "arm/intreadwrite.h" > -#elif ARCH_AVR32 > +#if ARCH_AVR32 > # include "avr32/intreadwrite.h" > #elif ARCH_MIPS > # include "mips/intreadwrite.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". OK, looks good to me.
Rémi Denis-Courmont: > Since the C11 support is required, those GCC versions can no longer be > supported anyhow. > --- > libavutil/arm/bswap.h | 20 -------- > libavutil/arm/intreadwrite.h | 91 ------------------------------------ > libavutil/intreadwrite.h | 4 +- > 3 files changed, 1 insertion(+), 114 deletions(-) > delete mode 100644 libavutil/arm/intreadwrite.h > > diff --git a/libavutil/arm/bswap.h b/libavutil/arm/bswap.h > index c3460e035d..48fefa4da3 100644 > --- a/libavutil/arm/bswap.h > +++ b/libavutil/arm/bswap.h > @@ -45,26 +45,6 @@ static av_always_inline av_const unsigned av_bswap16(unsigned x) > return y; > } > #endif > - > -#if AV_GCC_VERSION_AT_MOST(4,4) > -#define av_bswap32 av_bswap32 > -static av_always_inline av_const uint32_t av_bswap32(uint32_t x) > -{ > - uint32_t y; > -#if HAVE_ARMV6_INLINE > - __asm__("rev %0, %1" : "=r"(y) : "r"(x)); > -#else > - uint32_t t; > - __asm__ ("eor %1, %2, %2, ror #16 \n\t" > - "bic %1, %1, #0xFF0000 \n\t" > - "mov %0, %2, ror #8 \n\t" > - "eor %0, %0, %1, lsr #8 \n\t" > - : "=r"(y), "=&r"(t) : "r"(x)); > -#endif /* HAVE_ARMV6_INLINE */ > - return y; > -} > -#endif /* AV_GCC_VERSION_AT_MOST(4,4) */ > - > #endif /* __ARMCC_VERSION */ > > #endif /* AVUTIL_ARM_BSWAP_H */ > diff --git a/libavutil/arm/intreadwrite.h b/libavutil/arm/intreadwrite.h > deleted file mode 100644 > index 60fc860cbb..0000000000 > --- a/libavutil/arm/intreadwrite.h > +++ /dev/null > @@ -1,91 +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_ARM_INTREADWRITE_H > -#define AVUTIL_ARM_INTREADWRITE_H > - > -#include <stdint.h> > -#include "config.h" > -#include "libavutil/attributes.h" > - > -#if HAVE_FAST_UNALIGNED && HAVE_INLINE_ASM && AV_GCC_VERSION_AT_MOST(4,6) > - > -#define AV_RN16 AV_RN16 > -static av_always_inline unsigned AV_RN16(const void *p) > -{ > - const uint8_t *q = p; > - unsigned v; > -#if AV_GCC_VERSION_AT_MOST(4,5) > - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(*(const uint16_t *)q)); > -#elif defined __thumb__ > - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(q[0]), "m"(q[1])); > -#else > - __asm__ ("ldrh %0, %1" : "=r"(v) : "Uq"(q[0]), "m"(q[1])); > -#endif > - return v; > -} > - > -#define AV_WN16 AV_WN16 > -static av_always_inline void AV_WN16(void *p, uint16_t v) > -{ > - __asm__ ("strh %1, %0" : "=m"(*(uint16_t *)p) : "r"(v)); > -} > - > -#define AV_RN32 AV_RN32 > -static av_always_inline uint32_t AV_RN32(const void *p) > -{ > - const struct __attribute__((packed)) { uint32_t v; } *q = p; > - uint32_t v; > - __asm__ ("ldr %0, %1" : "=r"(v) : "m"(*q)); > - return v; > -} > - > -#define AV_WN32 AV_WN32 > -static av_always_inline void AV_WN32(void *p, uint32_t v) > -{ > - __asm__ ("str %1, %0" : "=m"(*(uint32_t *)p) : "r"(v)); > -} > - > -#if HAVE_ASM_MOD_Q > - > -#define AV_RN64 AV_RN64 > -static av_always_inline uint64_t AV_RN64(const void *p) > -{ > - const struct __attribute__((packed)) { uint32_t v; } *q = p; > - uint64_t v; > - __asm__ ("ldr %Q0, %1 \n\t" > - "ldr %R0, %2 \n\t" > - : "=&r"(v) > - : "m"(q[0]), "m"(q[1])); > - return v; > -} > - > -#define AV_WN64 AV_WN64 > -static av_always_inline void AV_WN64(void *p, uint64_t v) > -{ > - __asm__ ("str %Q2, %0 \n\t" > - "str %R2, %1 \n\t" > - : "=m"(*(uint32_t*)p), "=m"(*((uint32_t*)p+1)) > - : "r"(v)); > -} > - > -#endif /* HAVE_ASM_MOD_Q */ > - > -#endif /* HAVE_INLINE_ASM */ > - > -#endif /* AVUTIL_ARM_INTREADWRITE_H */ > diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h > index d0a5773b54..ccdbf79b74 100644 > --- a/libavutil/intreadwrite.h > +++ b/libavutil/intreadwrite.h > @@ -64,9 +64,7 @@ typedef union { > > #include "config.h" > > -#if ARCH_ARM > -# include "arm/intreadwrite.h" > -#elif ARCH_AVR32 > +#if ARCH_AVR32 > # include "avr32/intreadwrite.h" > #elif ARCH_MIPS > # include "mips/intreadwrite.h" FYI: Clang pretends to be GCC 4.4 here. (Also: IIRC Clang actually parses inline assembler and optimize it; neither of these points means that I am against your patch.) - Andreas
Le lauantaina 8. kesäkuuta 2024, 0.26.21 EEST Andreas Rheinhardt a écrit :
> (Also: IIRC Clang actually parses inline assembler and optimize it;
That sounds sketchy. While FFmpeg only uses inline assembler as intrinsic
ersatz, other projects can and definitely do use them for weirder stuff. That
will fail miserably if the compiler modifies the sequence.
The Linux kernel for instance pushes/pops sections or patches the byte
sequence at load time or even run time. Some code also assumes exact byte
offsets between instructions (not sure Linux specifically). So, AFAICT, the only
thing the compiler can sanely do is move, and maybe eliminate/replicate, the
entire assembler block.
Rémi Denis-Courmont: > Le lauantaina 8. kesäkuuta 2024, 0.26.21 EEST Andreas Rheinhardt a écrit : >> (Also: IIRC Clang actually parses inline assembler and optimize it; > > That sounds sketchy. While FFmpeg only uses inline assembler as intrinsic > ersatz, other projects can and definitely do use them for weirder stuff. That > will fail miserably if the compiler modifies the sequence. See https://godbolt.org/z/ozeGoWzK7: Clang is smart enough to know that the x86-inline asm version of av_bswap32 is self-inverse. (It can also evaluate it for constants at compile-time.) > > The Linux kernel for instance pushes/pops sections or patches the byte > sequence at load time or even run time. Some code also assumes exact byte > offsets between instructions (not sure Linux specifically). So, AFAICT, the only > thing the compiler can sanely do is move, and maybe eliminate/replicate, the > entire assembler block. >
Le lauantaina 8. kesäkuuta 2024, 9.23.48 EEST Andreas Rheinhardt a écrit : > Rémi Denis-Courmont: > > Le lauantaina 8. kesäkuuta 2024, 0.26.21 EEST Andreas Rheinhardt a écrit : > >> (Also: IIRC Clang actually parses inline assembler and optimize it; > > > > That sounds sketchy. While FFmpeg only uses inline assembler as intrinsic > > ersatz, other projects can and definitely do use them for weirder stuff. > > That will fail miserably if the compiler modifies the sequence. > > See https://godbolt.org/z/ozeGoWzK7: Clang is smart enough to know that > the x86-inline asm version of av_bswap32 is self-inverse. (It can also > evaluate it for constants at compile-time.) It seems that they have some heuristics not to touch assembler that they are not sure that they "completely" understand. If you add just a dummy numeric label, the optimisation is not done. My preference would be to use builtin_bswap (as riscv/ does) on all architectures on GCC and Clang, and keep the bespoke C for other compilers and external header inclusion.
Rémi Denis-Courmont: > Le lauantaina 8. kesäkuuta 2024, 9.23.48 EEST Andreas Rheinhardt a écrit : >> Rémi Denis-Courmont: >>> Le lauantaina 8. kesäkuuta 2024, 0.26.21 EEST Andreas Rheinhardt a écrit : >>>> (Also: IIRC Clang actually parses inline assembler and optimize it; >>> >>> That sounds sketchy. While FFmpeg only uses inline assembler as intrinsic >>> ersatz, other projects can and definitely do use them for weirder stuff. >>> That will fail miserably if the compiler modifies the sequence. >> >> See https://godbolt.org/z/ozeGoWzK7: Clang is smart enough to know that >> the x86-inline asm version of av_bswap32 is self-inverse. (It can also >> evaluate it for constants at compile-time.) > > It seems that they have some heuristics not to touch assembler that they are > not sure that they "completely" understand. If you add just a dummy numeric > label, the optimisation is not done. > > My preference would be to use builtin_bswap (as riscv/ does) on all > architectures on GCC and Clang, and keep the bespoke C for other compilers and > external header inclusion. > Why would you not want to use the compiler builtins for external users? - Andreas
Le lauantaina 8. kesäkuuta 2024, 16.41.13 EEST Andreas Rheinhardt a écrit : > > My preference would be to use builtin_bswap (as riscv/ does) on all > > architectures on GCC and Clang, and keep the bespoke C for other compilers > > and external header inclusion. > > Why would you not want to use the compiler builtins for external users? Assuming that the definitions are in the common headers, then they can be exposed to external users too, true enough. I was in the incorrect mindset that they would be in the arch-specific private headers.
diff --git a/libavutil/arm/bswap.h b/libavutil/arm/bswap.h index c3460e035d..48fefa4da3 100644 --- a/libavutil/arm/bswap.h +++ b/libavutil/arm/bswap.h @@ -45,26 +45,6 @@ static av_always_inline av_const unsigned av_bswap16(unsigned x) return y; } #endif - -#if AV_GCC_VERSION_AT_MOST(4,4) -#define av_bswap32 av_bswap32 -static av_always_inline av_const uint32_t av_bswap32(uint32_t x) -{ - uint32_t y; -#if HAVE_ARMV6_INLINE - __asm__("rev %0, %1" : "=r"(y) : "r"(x)); -#else - uint32_t t; - __asm__ ("eor %1, %2, %2, ror #16 \n\t" - "bic %1, %1, #0xFF0000 \n\t" - "mov %0, %2, ror #8 \n\t" - "eor %0, %0, %1, lsr #8 \n\t" - : "=r"(y), "=&r"(t) : "r"(x)); -#endif /* HAVE_ARMV6_INLINE */ - return y; -} -#endif /* AV_GCC_VERSION_AT_MOST(4,4) */ - #endif /* __ARMCC_VERSION */ #endif /* AVUTIL_ARM_BSWAP_H */ diff --git a/libavutil/arm/intreadwrite.h b/libavutil/arm/intreadwrite.h deleted file mode 100644 index 60fc860cbb..0000000000 --- a/libavutil/arm/intreadwrite.h +++ /dev/null @@ -1,91 +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_ARM_INTREADWRITE_H -#define AVUTIL_ARM_INTREADWRITE_H - -#include <stdint.h> -#include "config.h" -#include "libavutil/attributes.h" - -#if HAVE_FAST_UNALIGNED && HAVE_INLINE_ASM && AV_GCC_VERSION_AT_MOST(4,6) - -#define AV_RN16 AV_RN16 -static av_always_inline unsigned AV_RN16(const void *p) -{ - const uint8_t *q = p; - unsigned v; -#if AV_GCC_VERSION_AT_MOST(4,5) - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(*(const uint16_t *)q)); -#elif defined __thumb__ - __asm__ ("ldrh %0, %1" : "=r"(v) : "m"(q[0]), "m"(q[1])); -#else - __asm__ ("ldrh %0, %1" : "=r"(v) : "Uq"(q[0]), "m"(q[1])); -#endif - return v; -} - -#define AV_WN16 AV_WN16 -static av_always_inline void AV_WN16(void *p, uint16_t v) -{ - __asm__ ("strh %1, %0" : "=m"(*(uint16_t *)p) : "r"(v)); -} - -#define AV_RN32 AV_RN32 -static av_always_inline uint32_t AV_RN32(const void *p) -{ - const struct __attribute__((packed)) { uint32_t v; } *q = p; - uint32_t v; - __asm__ ("ldr %0, %1" : "=r"(v) : "m"(*q)); - return v; -} - -#define AV_WN32 AV_WN32 -static av_always_inline void AV_WN32(void *p, uint32_t v) -{ - __asm__ ("str %1, %0" : "=m"(*(uint32_t *)p) : "r"(v)); -} - -#if HAVE_ASM_MOD_Q - -#define AV_RN64 AV_RN64 -static av_always_inline uint64_t AV_RN64(const void *p) -{ - const struct __attribute__((packed)) { uint32_t v; } *q = p; - uint64_t v; - __asm__ ("ldr %Q0, %1 \n\t" - "ldr %R0, %2 \n\t" - : "=&r"(v) - : "m"(q[0]), "m"(q[1])); - return v; -} - -#define AV_WN64 AV_WN64 -static av_always_inline void AV_WN64(void *p, uint64_t v) -{ - __asm__ ("str %Q2, %0 \n\t" - "str %R2, %1 \n\t" - : "=m"(*(uint32_t*)p), "=m"(*((uint32_t*)p+1)) - : "r"(v)); -} - -#endif /* HAVE_ASM_MOD_Q */ - -#endif /* HAVE_INLINE_ASM */ - -#endif /* AVUTIL_ARM_INTREADWRITE_H */ diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h index d0a5773b54..ccdbf79b74 100644 --- a/libavutil/intreadwrite.h +++ b/libavutil/intreadwrite.h @@ -64,9 +64,7 @@ typedef union { #include "config.h" -#if ARCH_ARM -# include "arm/intreadwrite.h" -#elif ARCH_AVR32 +#if ARCH_AVR32 # include "avr32/intreadwrite.h" #elif ARCH_MIPS # include "mips/intreadwrite.h"