Skip to content

Remove extern "C" wrapping and fix format specifiers for ARM embedded builds #18000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
meta-codesync merged 1 commit into from
Mar 10, 2026
Merged

Remove extern "C" wrapping and fix format specifiers for ARM embedded builds #18000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions backends/cortex_m/ops/cmsis_scratch_buffer_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,8 @@
*/
#pragma once

#include "cortex_m_ops_common.h"
extern "C" {
#include "arm_nnfunctions.h"
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that cortex_m_ops_common.h includes CMSIS-NN headers, this file’s direct #include "arm_nnfunctions.h" is redundant and also puts CMSIS includes before the common header (which may introduce ordering issues depending on CMSIS header dependencies). Prefer relying on cortex_m_ops_common.h alone, or include it first and drop the standalone CMSIS include.

Suggested change
#include "arm_nnfunctions.h"

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@rascani rascani Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed OOB, we typically follow the Google C++ style guide, which recommends against transitive includes.

}
#include "cortex_m_ops_common.h"

namespace cortex_m {
namespace native {
Expand Down
33 changes: 17 additions & 16 deletions backends/cortex_m/ops/cortex_m_ops_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@
#include <executorch/kernels/portable/cpu/util/kernel_ops_util.h>
#include <executorch/runtime/platform/assert.h>

#include <cinttypes>
#include <limits>
#include <optional>

extern "C" {
#include "arm_nn_types.h"
}
#include "arm_nnfunctions.h"

Comment on lines +19 to 25
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description mentions consolidating CMSIS-NN includes by adding arm_nnfunctions.h to cortex_m_ops_common.h for transitive inclusion, but cortex_m_ops_common.h currently only includes arm_nn_types.h. Either update the PR description to reflect the current approach (explicit includes per op file) or actually add the include to match the described change.

Copilot uses AI. Check for mistakes.
using Tensor = torch::executor::Tensor;
using ScalarType = executorch::aten::ScalarType;
Expand All @@ -47,19 +47,19 @@ inline void validate_cmsis_nn_tensor_requirements(
// Basic dtype validation
ET_CHECK_MSG(
input1.scalar_type() == expected_dtype,
"Input1 dtype must be %hhd, got %hhd",
expected_dtype,
input1.scalar_type());
"Input1 dtype must be %d, got %d",
static_cast<int>(expected_dtype),
static_cast<int>(input1.scalar_type()));
ET_CHECK_MSG(
input2.scalar_type() == expected_dtype,
"Input2 dtype must be %hhd, got %hhd",
expected_dtype,
input2.scalar_type());
"Input2 dtype must be %d, got %d",
static_cast<int>(expected_dtype),
static_cast<int>(input2.scalar_type()));
ET_CHECK_MSG(
output.scalar_type() == expected_dtype,
"Output dtype must be %hhd, got %hhd",
expected_dtype,
output.scalar_type());
"Output dtype must be %d, got %d",
static_cast<int>(expected_dtype),
static_cast<int>(output.scalar_type()));
if (require_same_sizes) {
ET_CHECK_MSG(
input1.sizes() == input2.sizes(),
Expand All @@ -78,16 +78,17 @@ inline void validate_single_quant_params(
const int64_t multiplier,
const int64_t shift,
const char* param_name) {
(void)zero_point;
ET_CHECK_MSG(
Comment on lines +81 to 82
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_single_quant_params takes zero_point but explicitly ignores it. Callers (e.g., validate_quantization_params / ops casting zero_point down to int32_t) therefore don't validate that zero points are within an expected/representable range, and the nearby doc comment claims that zero points are checked. Consider either validating zero_point (e.g., int8 or at least int32 range, consistent with CMSIS/AOT expectations) or removing the parameter/updating the contract to avoid misleading validation.

Suggested change
(void)zero_point;
ET_CHECK_MSG(
ET_CHECK_MSG(
zero_point >= std::numeric_limits<int8_t>::min() &&
zero_point <= std::numeric_limits<int8_t>::max(),
"%s zero_point must be in int8 range [-128, 127] [Value: %" PRIi64 "]",
param_name,
zero_point);
ET_CHECK_MSG(

Copilot uses AI. Check for mistakes.
multiplier >= std::numeric_limits<int32_t>::min() &&
multiplier <= std::numeric_limits<int32_t>::max(),
"%s multiplier must be in int32 range [Value: %d]",
"%s multiplier must be in int32 range [Value: %" PRIi64 "]",
param_name,
multiplier);
Comment on lines 82 to 87
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In validate_single_quant_params, the zero_point parameter is unused. If this header is compiled with -Wall, this can trigger -Wunused-parameter (often fatal with -Werror on embedded toolchains). Consider removing the parameter, marking it ET_UNUSED, or implementing the intended zero-point range validation.

Copilot uses AI. Check for mistakes.

ET_CHECK_MSG(
shift >= -31 && shift <= 31,
"%s shift must be in range [-31, 31] [Value: %d]",
"%s shift must be in range [-31, 31] [Value: %" PRIi64 "]",
param_name,
shift);
}
Expand Down Expand Up @@ -172,7 +173,7 @@ inline bool check_int32_within_range(
value > std::numeric_limits<int32_t>::max()) {
ET_LOG(
Error,
"%s: %s value (%ld) exceeds int32_t range",
"%s: %s value (%" PRIi64 ") exceeds int32_t range",
op_name,
value_name,
value);
Expand Down Expand Up @@ -354,14 +355,14 @@ inline bool validate_per_channel_quant_params(
if (multipliers[i] <= ARM_NN_Q31_MIN || multipliers[i] > ARM_NN_Q31_MAX) {
ET_LOG(
Error,
"weight_multiplier[%d] out of CMSIS-NN range: %d",
"weight_multiplier[%d] out of CMSIS-NN range: %" PRIi64,
i,
multipliers[i]);
return false;
}
// Shift: {-31, 30} for arm_nn_requantize
if (shifts[i] < -31 || shifts[i] > 30) {
ET_LOG(Error, "weight_shift[%d] out of range: %d", i, shifts[i]);
ET_LOG(Error, "weight_shift[%d] out of range: %" PRIi64, i, shifts[i]);
return false;
}
}
Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_maximum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@

#include "cortex_m_ops_common.h"

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_minimum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@

#include "cortex_m_ops_common.h"

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_pad.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_quantized_add.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@

#include "cortex_m_ops_common.h"

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {
using KernelRuntimeContext = torch::executor::KernelRuntimeContext;
Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_avg_pool2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_conv2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_depthwise_conv2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_linear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {
using KernelRuntimeContext = torch::executor::KernelRuntimeContext;
Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_max_pool2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_quantized_mul.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@

#include "cortex_m_ops_common.h"

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {
namespace {
Expand Down
4 changes: 0 additions & 4 deletions backends/cortex_m/ops/op_quantized_transpose_conv2d.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@

#include "cortex_m_ops_common.h"

extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_softmax.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@
#include <cstdint>
#include <limits>

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
5 changes: 0 additions & 5 deletions backends/cortex_m/ops/op_transpose.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@
#include <limits>
#include <vector>

// Include CMSIS-NN headers with C linkage
extern "C" {
#include "arm_nnfunctions.h"
}

namespace cortex_m {
namespace native {

Expand Down
Loading