ARM64 implements SDL_Swap16/32

[SDLBugzilla]

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• diff (patch, text/plain, 2020-12-26 19:33:25 +0000, 1024 bytes)
• diff (patch, text/plain, 2020-12-28 06:09:40 +0000, 1026 bytes)

Reported in version: HG 2.1
Reported for operating system, platform: Other, ARM

Comments on the original bug report:

On 2020-12-26 19:33:25 +0000, David Carlier wrote:

Created attachment 4611
diff

Usage of halfword and word size instructions.

On 2020-12-28 04:21:17 +0000, Ozkan Sezer wrote:

Patch applied as https://hg.libsdl.org/SDL/rev/055b48432d03
Thanks.

On 2020-12-28 04:35:42 +0000, Ozkan Sezer wrote:

David Carlier: This patch resulted in multiple warnings in android
buildbot, can you have a look?
https://buildbot.libsdl.org/#/builders/24/builds/560

On 2020-12-28 05:04:02 +0000, Ozkan Sezer wrote:

Reverted the patch for now.

On 2020-12-28 06:09:40 +0000, David Carlier wrote:

Created attachment 4615
diff

On 2020-12-28 06:10:01 +0000, David Carlier wrote:

Attached a new version with register constraints.

On 2020-12-28 08:55:40 +0000, Ozkan Sezer wrote:

Patch applied as https://hg.libsdl.org/SDL/rev/459bb41a54b6
Closing as fixed.

[sezero]

@devnexen: I have a bug report at de4me/SDL-1.2-xcode@d750278
that the aarch64 Swap16 version is broken:
de4me/SDL-1.2-xcode@d750278#commitcomment-60831881
I guess maybe the correct version should be like:

static __inline__ Uint16 SDL_Swap16(Uint32 x)
{
	__asm__("rev16 %1, %0" : "=r"(x) : "r"(x));
	return (Uint16)x;
}

[sezero]

Or, should we just remove the aarch64 asm now that we handle things using __builtin_swapXX?

[sezero]

Or, should we just remove the aarch64 asm now that we handle things using __builtin_swapXX?

Well, I believe the safest way is removing the aarch64 asm.

This one is for SDL2 - suggest for 2.0.18:

diff --git a/include/SDL_endian.h b/include/SDL_endian.h
index 3306003..4ff7e03 100644
--- a/include/SDL_endian.h
+++ b/include/SDL_endian.h
@@ -135,13 +135,6 @@ SDL_Swap16(Uint16 x)
   __asm__("rlwimi %0,%2,8,16,23": "=&r"(result):"0"(x >> 8), "r"(x));
     return (Uint16)result;
 }
-#elif defined(__aarch64__)
-SDL_FORCE_INLINE Uint16
-SDL_Swap16(Uint16 x)
-{
-  __asm__("rev16 %w1, %w0" : "=r"(x) : "r"(x));
-  return x;
-}
 #elif (defined(__m68k__) && !defined(__mcoldfire__))
 SDL_FORCE_INLINE Uint16
 SDL_Swap16(Uint16 x)
@@ -193,13 +186,6 @@ SDL_Swap32(Uint32 x)
   __asm__("rlwimi %0,%2,24,0,7"  : "=&r"(result): "0" (result), "r"(x));
     return result;
 }
-#elif defined(__aarch64__)
-SDL_FORCE_INLINE Uint32
-SDL_Swap32(Uint32 x)
-{
-  __asm__("rev %w1, %w0": "=r"(x):"r"(x));
-  return x;
-}
 #elif (defined(__m68k__) && !defined(__mcoldfire__))
 SDL_FORCE_INLINE Uint32
 SDL_Swap32(Uint32 x)

And this one is for SDL-1.2, and considering aarch64 is new enough all of
gcc/clang versions supporting it must have __builtin_swapXX:

diff --git a/include/SDL_endian.h b/include/SDL_endian.h
index 72f257b..9b11987 100644
--- a/include/SDL_endian.h
+++ b/include/SDL_endian.h
@@ -94,16 +94,10 @@ static __inline__ Uint16 SDL_Swap16(Uint16 x)
 	__asm__("rlwimi %0,%2,8,16,23" : "=&r" (result) : "0" (x >> 8), "r" (x));
 	return (Uint16)result;
 }
-#elif defined(__APPLE__) && defined(__aarch64__)
-static __inline__ Uint16 SDL_Swap16(Uint16 x)
-{
-	return __builtin_bswap16(x);
-}
 #elif defined(__GNUC__) && defined(__aarch64__)
 static __inline__ Uint16 SDL_Swap16(Uint16 x)
 {
-	__asm__("rev16 %w1, %w0" : "=r"(x) : "r"(x));
-	return x;
+	return __builtin_bswap16(x);
 }
 #elif defined(__GNUC__) && (defined(__m68k__) && !defined(__mcoldfire__))
 static __inline__ Uint16 SDL_Swap16(Uint16 x)
@@ -146,16 +140,10 @@ static __inline__ Uint32 SDL_Swap32(Uint32 x)
 	__asm__("rlwimi %0,%2,24,0,7"   : "=&r" (result) : "0" (result), "r" (x));
 	return result;
 }
-#elif defined(__APPLE__) && defined(__aarch64__)
-static __inline__ Uint32 SDL_Swap32(Uint32 x)
-{
-	return __builtin_bswap32(x);
-}
 #elif defined(__GNUC__) && defined(__aarch64__)
 static __inline__ Uint32 SDL_Swap32(Uint32 x)
 {
-	__asm__("rev %w1, %w0": "=r"(x):"r"(x));
-	return x;
+	return __builtin_bswap32(x);
 }
 #elif defined(__GNUC__) && (defined(__m68k__) && !defined(__mcoldfire__))
 static __inline__ Uint32 SDL_Swap32(Uint32 x)
@@ -175,7 +163,7 @@ static __inline__ Uint32 SDL_Swap32(Uint32 x) {
 }
 #endif
 
-#ifdef SDL_HAS_64BIT_TYPE
+#ifdef SDL_HAS_64BIT_TYPE /**/
 #if defined(__GNUC__) && defined(__i386__) && \
    !(__GNUC__ == 2 && __GNUC_MINOR__ <= 95 /* broken gcc version */)
 static __inline__ Uint64 SDL_Swap64(Uint64 x)
@@ -190,7 +178,7 @@ static __inline__ Uint64 SDL_Swap64(Uint64 x)
 	        : "0"  (v.s.a),  "1" (v.s.b));
 	return v.u;
 }
-#elif defined(__APPLE__) && defined(__aarch64__)
+#elif defined(__GNUC__) && defined(__aarch64__)
 static __inline__ Uint64 SDL_Swap64(Uint64 x)
 {
 	return __builtin_bswap64(x);

OK to apply?

[devnexen]

It is better if toolchains's builtin are supported indeed, even tough I know better now the m1 specifics vs the common arm64 are still a bit of mistery :)

[sezero]

Patches applied to SDL-12 and sdl12-compat. Waiting for approval to apply to SDL2.

[icculus]

Apply this to SDL2, please.

[sezero]

Apply this to SDL2, please.

Done: eb39e20