Address remaining PR #101 review: improved Makefile detection, lighter NEON dcomp

- Makefile.common: validate BLITTER_SIMD value, detect via $(platform)/$(ARCH)
  variables for cross-compilation, keep uname -m as native fallback only
- blitter_simd_neon.c: replace vcreate_u8 weights + vpadd chain with
  vshr_n_u8 + lane extraction for fewer instructions in dcomp
- test_blitter_simd.c: fix duplicate lines from rebase

Co-Authored-By: Claude Opus 4.6 <[email protected]>

Author: JoeMatt

Makefile.common

@@ -49,38 +49,57 @@ SOURCES_C :=  \
 	$(CORE_DIR)/src/wavetable.c
 
 # SIMD-accelerated blitter operations: select arch-specific implementation.
-# Override with BLITTER_SIMD=scalar|sse2|neon for cross-compilation.
-# Default to portable scalar fallback.
+# BLITTER_SIMD may be set explicitly to one of: scalar, sse2, neon.
 BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_scalar.c
 
-ifdef BLITTER_SIMD
-   # Explicit override for cross-compilation or custom builds
-   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_$(BLITTER_SIMD).c
-else
-   # Auto-detect from host architecture (native builds only)
-   # x86/x64: use SSE2 (baseline for all x86_64, available since Pentium 4)
-   ifneq (,$(filter x86_64 x86 i686 i386,$(shell uname -m 2>/dev/null)))
-      BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
-   endif
-   # MSYS2/MinGW on x86_64
-   ifneq (,$(filter MINGW64%,$(MSYSTEM)))
-      BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
-   endif
-   # ARM64 (AArch64): NEON is always available
-   ifneq (,$(filter aarch64 arm64,$(shell uname -m 2>/dev/null)))
-      BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
-   endif
+ifneq ($(BLITTER_SIMD),)
+ifeq (,$(filter scalar sse2 neon,$(BLITTER_SIMD)))
+$(error Unsupported BLITTER_SIMD '$(BLITTER_SIMD)'; expected one of: scalar sse2 neon)
+endif
 endif
 
-# Platform-based overrides (cross-compilation targets)
+ifeq ($(BLITTER_SIMD),sse2)
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
+else ifeq ($(BLITTER_SIMD),neon)
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
+else ifeq ($(BLITTER_SIMD),scalar)
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_scalar.c
+else
+# ARM targets: prefer NEON when the target configuration says it is available.
+ifeq ($(HAVE_NEON), 1)
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
+endif
 ifneq (,$(filter ios-arm64 tvos-arm64,$(platform)))
    BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
 endif
-# ARMv7 with NEON (set by Makefile for classic_armv7 targets)
-ifeq ($(HAVE_NEON), 1)
+ifneq (,$(filter aarch64 arm64 arm,$(ARCH)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
+endif
+ifneq (,$(filter arm64 armv8% armv7% armhf arm,$(platform)))
    BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
 endif
 
+# x86/x64 targets: use SSE2.
+ifneq (,$(filter x86_64 x86 i686 i386,$(ARCH)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
+endif
+ifneq (,$(filter x86_64 x86 i686 i386 win-x64 win32,$(platform)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
+endif
+# MSYS2/MinGW on x86_64
+ifneq (,$(filter MINGW64%,$(MSYSTEM)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
+endif
+
+# Native build fallback: auto-detect from host architecture
+ifneq (,$(filter x86_64 x86 i686 i386,$(shell uname -m 2>/dev/null)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_sse2.c
+endif
+ifneq (,$(filter aarch64 arm64,$(shell uname -m 2>/dev/null)))
+   BLITTER_SIMD_SRC := $(CORE_DIR)/src/blitter_simd_neon.c
+endif
+endif
+
 SOURCES_C += $(BLITTER_SIMD_SRC)
 
 ifneq ($(STATIC_LINKING), 1)

src/blitter_simd_neon.c

@@ -54,17 +54,20 @@ static uint8_t neon_dcomp(uint64_t patd, uint64_t srcd, uint64_t dstd, bool cmpd
    uint8x8_t vzero = vdup_n_u8(0);
    uint8x8_t vcmp = vceq_u8(vxor, vzero);  /* 0xFF where equal, 0 otherwise */
 
-   /* Extract one bit per byte using power-of-2 weights.
-    * vcreate avoids a static const load on every call. */
-   uint8x8_t vw = vcreate_u8(0x8040201008040201ULL);
-   uint8x8_t vbits = vand_u8(vcmp, vw);
-
-   /* Pairwise horizontal add: 8 -> 4 -> 2 -> 1 */
-   uint8x8_t sum1 = vpadd_u8(vbits, vbits);
-   uint8x8_t sum2 = vpadd_u8(sum1, sum1);
-   uint8x8_t sum3 = vpadd_u8(sum2, sum2);
-
-   return vget_lane_u8(sum3, 0);
+   /* Convert compare lanes from 0xFF/0x00 to 1/0, then pack into
+    * the result byte via lane extraction — avoids weights load
+    * and vpadd chain. */
+   uint8x8_t vbits = vshr_n_u8(vcmp, 7);
+
+   return (uint8_t)(
+      (vget_lane_u8(vbits, 0) << 0) |
+      (vget_lane_u8(vbits, 1) << 1) |
+      (vget_lane_u8(vbits, 2) << 2) |
+      (vget_lane_u8(vbits, 3) << 3) |
+      (vget_lane_u8(vbits, 4) << 4) |
+      (vget_lane_u8(vbits, 5) << 5) |
+      (vget_lane_u8(vbits, 6) << 6) |
+      (vget_lane_u8(vbits, 7) << 7));
 }
 
 /* Z-buffer Comparator — NEON

test/test_blitter_simd.c

@@ -447,5 +447,3 @@ int main(int argc, char *argv[])
 
    return failures > 0 ? 1 : 0;
 }
-? 1 : 0;
-}