# aom: {,highbd\_}intrapred\_neon.c: Fix unaligned accesses in z2 preds

**URL:** <https://discourse.libsdl.org/t/aom-highbd-intrapred-neon-c-fix-unaligned-accesses-in-z2-preds/52955>\
**Category:** SDL Commits\
**Created:** [July 20, 2024, 11:28am UTC](https://discourse.libsdl.org/t/aom-highbd-intrapred-neon-c-fix-unaligned-accesses-in-z2-preds/52955 "2024-07-20T11:28:33Z")\
**Posts on this page:** 1\
**Page:** 1

<div class="post-metadata">

**Author:** ![SDLPushPostBot](https://discourse.libsdl.org/user_avatar/discourse.libsdl.org/sdlpushpostbot/32/4370_2.png) [@SDLPushPostBot](https://discourse.libsdl.org/u/SDLPushPostBot)\
**Post date:** [July 20, 2024, 11:28am UTC](https://discourse.libsdl.org/t/aom-highbd-intrapred-neon-c-fix-unaligned-accesses-in-z2-preds/52955/1 "2024-07-20T11:28:33Z")

</div>

> <https://github.com/libsdl-org/aom/commit/93b8eee4f428675195c5c76e8da719ff50c2a01c>
>
> The z2 predictors load 32-bit chunks of data at variable offsets,
> however on AAr…ch32 hardware with alignment checks enabled this can cause
> alignment faults since the data is not guaranteed to be 32-bit aligned.
> 
> To work around this we can make use of the load\_unaligned\_u8\_4x1 in some
> cases and in others simply memcpy to avoid making any alignment
> guarantees for the data being loaded.
> 
> This also reverts commit 5a46d2961fb233c8f099a7bc18a7a54c8883813b,
> re-enabling the predictors for 32-bit Arm platforms.
> 
> Bug: b/349428506
> Change-Id: Ib15a7993e50b4bb6dd2eb4de95fba1dc33d3cd3f

```diff
From 93b8eee4f428675195c5c76e8da719ff50c2a01c Mon Sep 17 00:00:00 2001
From: George Steed <[EMAIL REDACTED]>
Date: Fri, 28 Jun 2024 10:39:49 +0100
Subject: [PATCH] {,highbd_}intrapred_neon.c: Fix unaligned accesses in z2
 preds

The z2 predictors load 32-bit chunks of data at variable offsets,
however on AArch32 hardware with alignment checks enabled this can cause
alignment faults since the data is not guaranteed to be 32-bit aligned.

To work around this we can make use of the load_unaligned_u8_4x1 in some
cases and in others simply memcpy to avoid making any alignment
guarantees for the data being loaded.

This also reverts commit 5a46d2961fb233c8f099a7bc18a7a54c8883813b,
re-enabling the predictors for 32-bit Arm platforms.

Bug: b/349428506
Change-Id: Ib15a7993e50b4bb6dd2eb4de95fba1dc33d3cd3f
---
 aom_dsp/arm/highbd_intrapred_neon.c | 32 +++++++++------------
 aom_dsp/arm/intrapred_neon.c | 7 ++---
 aom_dsp/arm/mem_neon.h | 26 +++++++++++++++++
 av1/common/av1_rtcd_defs.pl | 14 ++-------
 test/dr_prediction_test.cc | 44 -----------------------------
 5 files changed, 43 insertions(+), 80 deletions(-)

diff --git a/aom_dsp/arm/highbd_intrapred_neon.c b/aom_dsp/arm/highbd_intrapred_neon.c
index 5e6118dc6f..71d133e814 100644
--- a/aom_dsp/arm/highbd_intrapred_neon.c
+++ b/aom_dsp/arm/highbd_intrapred_neon.c
@@ -16,6 +16,7 @@
 #include "config/av1_rtcd.h"
 
 #include "aom/aom_integer.h"
+#include "aom_dsp/arm/mem_neon.h"
 #include "aom_dsp/arm/sum_neon.h"
 #include "aom_dsp/arm/transpose_neon.h"
 #include "aom_dsp/intrapred_common.h"
@@ -1604,8 +1605,6 @@ static AOM_FORCE_INLINE uint16x8_t highbd_dr_prediction_z2_tbl_left_x8_from_x16(
 }
 #endif // AOM_ARCH_AARCH64
 
-// TODO(aomedia:349428506): enable this for armv7 after SIGBUS is fixed.
-#if AOM_ARCH_AARCH64
 static AOM_FORCE_INLINE uint16x4x2_t highbd_dr_prediction_z2_gather_left_x4(
     const uint16_t *left, const int16x4_t indices, int n) {
   assert(n > 0);
@@ -1625,13 +1624,13 @@ static AOM_FORCE_INLINE uint16x4x2_t highbd_dr_prediction_z2_gather_left_x4(
 
   // At time of writing both Clang and GCC produced better code with these
   // nested if-statements compared to a switch statement with fallthrough.
- ret0_u32 = vld1_lane_u32((const uint32_t *)(left + idx0), ret0_u32, 0);
+ load_unaligned_u32_2x1_lane(ret0_u32, left + idx0, 0);
   if (n > 1) {
- ret0_u32 = vld1_lane_u32((const uint32_t *)(left + idx1), ret0_u32, 1);
+ load_unaligned_u32_2x1_lane(ret0_u32, left + idx1, 1);
     if (n > 2) {
- ret1_u32 = vld1_lane_u32((const uint32_t *)(left + idx2), ret1_u32, 0);
+ load_unaligned_u32_2x1_lane(ret1_u32, left + idx2, 0);
       if (n > 3) {
- ret1_u32 = vld1_lane_u32((const uint32_t *)(left + idx3), ret1_u32, 1);
+ load_unaligned_u32_2x1_lane(ret1_u32, left + idx3, 1);
       }
     }
   }
@@ -1665,25 +1664,21 @@ static AOM_FORCE_INLINE uint16x8x2_t highbd_dr_prediction_z2_gather_left_x8(
 
   // At time of writing both Clang and GCC produced better code with these
   // nested if-statements compared to a switch statement with fallthrough.
- ret0_u32 = vld1q_lane_u32((const uint32_t *)(left + idx0), ret0_u32, 0);
+ load_unaligned_u32_4x1_lane(ret0_u32, left + idx0, 0);
   if (n > 1) {
- ret0_u32 = vld1q_lane_u32((const uint32_t *)(left + idx1), ret0_u32, 1);
+ load_unaligned_u32_4x1_lane(ret0_u32, left + idx1, 1);
     if (n > 2) {
- ret0_u32 = vld1q_lane_u32((const uint32_t *)(left + idx2), ret0_u32, 2);
+ load_unaligned_u32_4x1_lane(ret0_u32, left + idx2, 2);
       if (n > 3) {
- ret0_u32 = vld1q_lane_u32((const uint32_t *)(left + idx3), ret0_u32, 3);
+ load_unaligned_u32_4x1_lane(ret0_u32, left + idx3, 3);
         if (n > 4) {
- ret1_u32 =
- vld1q_lane_u32((const uint32_t *)(left + idx4), ret1_u32, 0);
+ load_unaligned_u32_4x1_lane(ret1_u32, left + idx4, 0);
           if (n > 5) {
- ret1_u32 =
- vld1q_lane_u32((const uint32_t *)(left + idx5), ret1_u32, 1);
+ load_unaligned_u32_4x1_lane(ret1_u32, left + idx5, 1);
             if (n > 6) {
- ret1_u32 =
- vld1q_lane_u32((const uint32_t *)(left + idx6), ret1_u32, 2);
+ load_unaligned_u32_4x1_lane(ret1_u32, left + idx6, 2);
               if (n > 7) {
- ret1_u32 = vld1q_lane_u32((const uint32_t *)(left + idx7),
- ret1_u32, 3);
+ load_unaligned_u32_4x1_lane(ret1_u32, left + idx7, 3);
               }
             }
           }
@@ -2475,7 +2470,6 @@ void av1_highbd_dr_prediction_z2_neon(uint16_t *dst, ptrdiff_t stride, int bw,
   assert(f != NULL);
   f(dst, stride, above, left, upsample_above, upsample_left, dx, dy, bd);
 }
-#endif // AOM_ARCH_AARCH64
 
 // -----------------------------------------------------------------------------
 // Z3
diff --git a/aom_dsp/arm/intrapred_neon.c b/aom_dsp/arm/intrapred_neon.c
index 561a9f76a1..7fd82a1b82 100644
--- a/aom_dsp/arm/intrapred_neon.c
+++ b/aom_dsp/arm/intrapred_neon.c
@@ -1488,8 +1488,6 @@ void av1_dr_prediction_z1_neon(uint8_t *dst, ptrdiff_t stride, int bw, int bh,
 
 /* ---------------------P R E D I C T I O N Z 2--------------------------- */
 
-// TODO(aomedia:349428506): enable this for armv7 after SIGBUS is fixed.
-#if AOM_ARCH_AARCH64
 #if !AOM_ARCH_AARCH64
 static DECLARE_ALIGNED(16, uint8_t, LoadMaskz2[4][16]) = {
   { 0xff, 0xff, 0xff, 0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
@@ -1514,8 +1512,8 @@ static AOM_FORCE_INLINE void dr_prediction_z2_Nx4_above_neon(
     *a1_x = vuzp_u8(v_tmp, vdup_n_u8(0)).val[1];
     *shift0 = vand_u16(vsub_u16(r6, ydx), vdup_n_u16(0x1f));
   } else {
- *a0_x = load_u8_4x1(above + base_x);
- *a1_x = load_u8_4x1(above + base_x + 1);
+ *a0_x = load_unaligned_u8_4x1(above + base_x);
+ *a1_x = load_unaligned_u8_4x1(above + base_x + 1);
     *shift0 = vand_u16(vhsub_u16(r6, ydx), vdup_n_u16(0x1f));
   }
 }
@@ -2040,7 +2038,6 @@ void av1_dr_prediction_z2_neon(uint8_t *dst, ptrdiff_t stride, int bw, int bh,
       break;
   }
 }
-#endif // AOM_ARCH_AARCH64
 
 /* ---------------------P R E D I C T I O N Z 3--------------------------- */
 
diff --git a/aom_dsp/arm/mem_neon.h b/aom_dsp/arm/mem_neon.h
index 41efd03632..9734f8bd52 100644
--- a/aom_dsp/arm/mem_neon.h
+++ b/aom_dsp/arm/mem_neon.h
@@ -949,6 +949,32 @@ static INLINE void load_s16_8x3(const int16_t *s, ptrdiff_t p,
   *s2 = vld1q_s16(s);
 }
 
+#if AOM_ARCH_AARCH64
+#define load_unaligned_u32_2x1_lane(v, p, lane) \
+ do { \
+ (v) = vld1_lane_u32((const uint32_t *)(p), (v), (lane)); \
+ } while (0)
+
+#define load_unaligned_u32_4x1_lane(v, p, lane) \
+ do { \
+ (v) = vld1q_lane_u32((const uint32_t *)(p), (v), (lane)); \
+ } while (0)
+#else
+#define load_unaligned_u32_2x1_lane(v, p, lane) \
+ do { \
+ uint32_t tmp; \
+ memcpy(&tmp, (p), 4); \
+ (v) = vset_lane_u32(tmp, (v), (lane)); \
+ } while (0)
+
+#define load_unaligned_u32_4x1_lane(v, p, lane) \
+ do { \
+ uint32_t tmp; \
+ memcpy(&tmp, (p), 4); \
+ (v) = vsetq_lane_u32(tmp, (v), (lane)); \
+ } while (0)
+#endif
+
 // Load 2 sets of 4 bytes when alignment is not guaranteed.
 static INLINE uint8x8_t load_unaligned_u8(const uint8_t *buf, int stride) {
   uint32_t a;
diff --git a/av1/common/av1_rtcd_defs.pl b/av1/common/av1_rtcd_defs.pl
index 284f0efe9f..5233325624 100644
--- a/av1/common/av1_rtcd_defs.pl
+++ b/av1/common/av1_rtcd_defs.pl
@@ -115,12 +115,7 @@ ()
 add_proto qw/void av1_dr_prediction_z1/, "uint8_t *dst, ptrdiff_t stride, int bw, int bh, const uint8_t *above, const uint8_t *left, int upsample_above, int dx, int dy";
 specialize qw/av1_dr_prediction_z1 sse4_1 avx2 neon/;
 add_proto qw/void av1_dr_prediction_z2/, "uint8_t *dst, ptrdiff_t stride, int bw, int bh, const uint8_t *above, const uint8_t *left, int upsample_above, int upsample_left, int dx, int dy";
-# TODO(aomedia:349428506): enable NEON for armv7 after SIGBUS is fixed.
-if (aom_config("AOM_ARCH_ARM") eq "yes" && aom_config("AOM_ARCH_AARCH64") eq "") {
- specialize qw/av1_dr_prediction_z2 sse4_1 avx2/;
-} else {
- specialize qw/av1_dr_prediction_z2 sse4_1 avx2 neon/;
-}
+specialize qw/av1_dr_prediction_z2 sse4_1 avx2 neon/;
 add_proto qw/void av1_dr_prediction_z3/, "uint8_t *dst, ptrdiff_t stride, int bw, int bh, const uint8_t *above, const uint8_t *left, int upsample_left, int dx, int dy";
 specialize qw/av1_dr_prediction_z3 sse4_1 avx2 neon/;
 
@@ -230,12 +225,7 @@ ()
   add_proto qw/void av1_highbd_dr_prediction_z1/, "uint16_t *dst, ptrdiff_t stride, int bw, int bh, const uint16_t *above, const uint16_t *left, int upsample_above, int dx, int dy, int bd";
   specialize qw/av1_highbd_dr_prediction_z1 avx2 neon/;
   add_proto qw/void av1_highbd_dr_prediction_z2/, "uint16_t *dst, ptrdiff_t stride, int bw, int bh, const uint16_t *above, const uint16_t *left, int upsample_above, int upsample_left, int dx, int dy, int bd";
- # TODO(aomedia:349428506): enable NEON for armv7 after SIGBUS is fixed.
- if (aom_config("AOM_ARCH_ARM") eq "yes" && aom_config("AOM_ARCH_AARCH64") eq "") {
- specialize qw/av1_highbd_dr_prediction_z2 avx2/;
- } else {
- specialize qw/av1_highbd_dr_prediction_z2 avx2 neon/;
- }
+ specialize qw/av1_highbd_dr_prediction_z2 avx2 neon/;
   add_proto qw/void av1_highbd_dr_prediction_z3/, "uint16_t *dst, ptrdiff_t stride, int bw, int bh, const uint16_t *above, const uint16_t *left, int upsample_left, int dx, int dy, int bd";
   specialize qw/av1_highbd_dr_prediction_z3 avx2 neon/;
 }
diff --git a/test/dr_prediction_test.cc b/test/dr_prediction_test.cc
index 20cf600320..0938a3db11 100644
--- a/test/dr_prediction_test.cc
+++ b/test/dr_prediction_test.cc
@@ -484,7 +484,6 @@ INSTANTIATE_TEST_SUITE_P(
 #endif // HAVE_AVX2
 
 #if HAVE_NEON
-#if AOM_ARCH_AARCH64
 INSTANTIATE_TEST_SUITE_P(
     NEON, LowbdDrPredTest,
     ::testing::Values(DrPredFunc<DrPred>(&z1_wrapper<av1_dr_prediction_z1_c>,
@@ -496,21 +495,8 @@ INSTANTIATE_TEST_SUITE_P(
                       DrPredFunc<DrPred>(&z3_wrapper<av1_dr_prediction_z3_c>,
                                          &z3_wrapper<av1_dr_prediction_z3_neon>,
                                          AOM_BITS_8, kZ3Start)));
-#else
-// TODO(aomedia:349428506): enable av1_highbd_dr_prediction_z2_neon for armv7
-// after SIGBUS is fixed.
-INSTANTIATE_TEST_SUITE_P(
- NEON, LowbdDrPredTest,
- ::testing::Values(DrPredFunc<DrPred>(&z1_wrapper<av1_dr_prediction_z1_c>,
- &z1_wrapper<av1_dr_prediction_z1_neon>,
- AOM_BITS_8, kZ1Start),
- DrPredFunc<DrPred>(&z3_wrapper<av1_dr_prediction_z3_c>,
- &z3_wrapper<av1_dr_prediction_z3_neon>,
- AOM_BITS_8, kZ3Start)));
-#endif
 
 #if CONFIG_AV1_HIGHBITDEPTH
-#if AOM_ARCH_AARCH64
 INSTANTIATE_TEST_SUITE_P(
     NEON, HighbdDrPredTest,
     ::testing::Values(DrPredFunc<DrPred_Hbd>(
@@ -549,36 +535,6 @@ INSTANTIATE_TEST_SUITE_P(
                           &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_c>,
                           &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_neon>,
                           AOM_BITS_12, kZ3Start)));
-#else // !AOM_ARCH_AARCH64
-// TODO(aomedia:349428506): enable av1_highbd_dr_prediction_z2_neon for armv7
-// after SIGBUS is fixed.
-INSTANTIATE_TEST_SUITE_P(
- NEON, HighbdDrPredTest,
- ::testing::Values(DrPredFunc<DrPred_Hbd>(
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_c>,
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_neon>,
- AOM_BITS_8, kZ1Start),
- DrPredFunc<DrPred_Hbd>(
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_c>,
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_neon>,
- AOM_BITS_10, kZ1Start),
- DrPredFunc<DrPred_Hbd>(
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_c>,
- &z1_wrapper_hbd<av1_highbd_dr_prediction_z1_neon>,
- AOM_BITS_12, kZ1Start),
- DrPredFunc<DrPred_Hbd>(
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_c>,
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_neon>,
- AOM_BITS_8, kZ3Start),
- DrPredFunc<DrPred_Hbd>(
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_c>,
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_neon>,
- AOM_BITS_10, kZ3Start),
- DrPredFunc<DrPred_Hbd>(
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_c>,
- &z3_wrapper_hbd<av1_highbd_dr_prediction_z3_neon>,
- AOM_BITS_12, kZ3Start)));
-#endif // AOM_ARCH_AARCH64
 #endif // CONFIG_AV1_HIGHBITDEPTH
 
 #endif // HAVE_NEON

```