ARM-software / ComputeLibrary

The Compute Library is a set of computer vision and machine learning functions optimised for both Arm CPUs and GPUs using SIMD technologies.
2.87k stars 783 forks source link

-Werror=maybe-uninitialized in src/core/NEON/kernels/arm_gemm/quantized.cpp #1121

Closed robertbcalhoun closed 4 months ago

robertbcalhoun commented 4 months ago

Output of 'strings libarm_compute.so | grep arm_compute_version': compiling from source, commit 505adb91 (tag: v24.06)

Platform: aarch64

Operating System: linux

Problem description: The following error is thrown when compiling under gcc 13:

In member function ‘int16x8_t arm_gemm::{anonymous}::row_sum_helpers::accumulate_masked_16(const T*, int16x8_t, uint64x2_t) [with T = signed char]’,
    inlined from ‘void arm_gemm::{anonymous}::row_sum_helpers::compute_some_rows(unsigned int, const T*, unsigned int, int32_t*, unsigned int, uint64x2_t, int32x4_t) [with unsigned int rows = 2; T = signed char]’ at src/core/NEON/kernels/arm_gemm/quantized.cpp:846:55,
    inlined from ‘void arm_gemm::compute_row_sums(const Requantize32&, unsigned int, unsigned int, const T*, unsigned int, int32_t*) [with T = signed char]’ at src/core/NEON/kernels/arm_gemm/quantized.cpp:998:48:
src/core/NEON/kernels/arm_gemm/quantized.cpp:925:31: error: ‘mask’ may be used uninitialized [-Werror=maybe-uninitialized]
  925 |         int8x16_t v = vandq_s8(vld1q_s8(ptr), vreinterpretq_s8_u64(mask));
      |                       ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/core/NEON/kernels/arm_gemm/quantized.cpp: In function ‘void arm_gemm::compute_row_sums(const Requantize32&, unsigned int, unsigned int, const T*, unsigned int, int32_t*) [with T = signed char]’:
src/core/NEON/kernels/arm_gemm/quantized.cpp:969:16: note: ‘mask’ was declared here
  969 |     uint64x2_t mask;
      |                ^~~~
In member function ‘int16x8_t arm_gemm::{anonymous}::row_sum_helpers::accumulate_masked_16(const T*, int16x8_t, uint64x2_t) [with T = unsigned char]’,
    inlined from ‘void arm_gemm::{anonymous}::row_sum_helpers::compute_some_rows(unsigned int, const T*, unsigned int, int32_t*, unsigned int, uint64x2_t, int32x4_t) [with unsigned int rows = 2; T = unsigned char]’ at src/core/NEON/kernels/arm_gemm/quantized.cpp:846:55,
    inlined from ‘void arm_gemm::compute_row_sums(const Requantize32&, unsigned int, unsigned int, const T*, unsigned int, int32_t*) [with T = unsigned char]’ at src/core/NEON/kernels/arm_gemm/quantized.cpp:998:48:
src/core/NEON/kernels/arm_gemm/quantized.cpp:931:32: error: ‘mask’ may be used uninitialized [-Werror=maybe-uninitialized]
  931 |         uint8x16_t v = vandq_u8(vld1q_u8(ptr), vreinterpretq_u8_u64(mask));
      |                        ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/core/NEON/kernels/arm_gemm/quantized.cpp: In function ‘void arm_gemm::compute_row_sums(const Requantize32&, unsigned int, unsigned int, const T*, unsigned int, int32_t*) [with T = unsigned char]’:
src/core/NEON/kernels/arm_gemm/quantized.cpp:969:16: note: ‘mask’ was declared here
  969 |     uint64x2_t mask;
      |                ^~~~
cc1plus: all warnings being treated as errors

I believe this is a false positive generated by lines 972..986, as the case statement does not handle the case where odds == 0.

    if (odds > 0 && odds <= 8) {
        /* 1-8 odds: mask in the low lane, 0 in the top */
        uint64_t maskval = (~0ULL) >> (8 * (8-odds));

        mask = vsetq_lane_u64(maskval, vdupq_n_u64(0), 0);

        mask_mode = 1;
    } else if (odds > 8) {
        /* 9-15 odds: mask in the top lane, all 1s in the bottom. */
        uint64_t maskval = (~0ULL) >> (8 * (16-odds));

        mask = vsetq_lane_u64(maskval, vdupq_n_u64(~0ULL), 1);

        mask_mode = 2;
    }

I believe the masking code will not be used when odds == 0. I addressed the warning by adding:

diff --git a/src/core/NEON/kernels/arm_gemm/quantized.cpp b/src/core/NEON/kernels/arm_gemm/quantized.cpp
index 6da9f4be0e..b06a4dfac5 100644
--- a/src/core/NEON/kernels/arm_gemm/quantized.cpp
+++ b/src/core/NEON/kernels/arm_gemm/quantized.cpp
@@ -983,6 +983,11 @@ void compute_row_sums(const Requantize32 &qp, unsigned int width, unsigned int h
         mask = vsetq_lane_u64(maskval, vdupq_n_u64(~0ULL), 1);

         mask_mode = 2;
+    } else {
+        /* suppress GCC -Werror=maybe-uninitialized (odds==0, won't be called) */
+        uint64_t maskval = (~0ULL);
+        mask = vsetq_lane_u64(maskval, vdupq_n_u64(0), 0);
+        mask_mode = 0;
     }

Not filing a PR, as I am not confident that this is correct.

robertbcalhoun commented 4 months ago

Compiler version:

host/bin/aarch64-buildroot-linux-gnu-g++ --version
aarch64-buildroot-linux-gnu-g++.br_real (Buildroot 2024.02.4) 13.3.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
morgolock commented 4 months ago

Hi @robertbcalhoun

The following patch fixes the problem https://review.mlplatform.org/c/ml/ComputeLibrary/+/11840

Hope this helps

robertbcalhoun commented 4 months ago

It does, thank you!