Skip to content
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.

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

RFC: Streamline implementation overrides #392

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MarekPikula
Copy link

Background and motivation

While preparing initial patches for RISC-V support (as mentioned in #368), I noticed that overriding function implementations requires a lot of manual copy-pasting, leading to code duplication and increased maintenance effort.

This PR proposes a streamlined approach to declaring function overrides. I applied it to two functions in Silk (silk_NSQ and silk_VQ_WMat_EC) for x86 to demonstrate its benefits and gather feedback on whether such a refactor would be accepted upstream.

If approved, I can extend this approach to all overridable functions across all architectures, including Celt (which seems to use a similar override mechanism – please correct me if I'm wrong).

Proposal

  1. To reduce code duplication, function declaration can be set in a single macro (inmain_overrides.h), which can also be used for function type definition for use in the implementation array (e.g., for silk_NSQ function, there is now SILK_NSQ_DECL(impl) macro, which can be used to declare the function for all backends).
  2. The arch argument in the main macro definition is moved to the front, allowing for the use of macro variadic arguments, reducing the copy-paste of argument lists for each implementation.
  3. To declare a single backend override, there is a new macro OVERRIDE_IMPL_SINGLE, which references the selected backend implementation.
  4. Accordingly, for the multi-backend override, there is a new macro OVERRIDE_IMPL_ARRAY, which addresses the override array created with OVERRIDE_IMPL_ARRAY_DECL.

This approach significantly reduces manual (and error-prone) copy-paste work, minimizes the risk of errors when developing new backends (e.g., RISC-V in our case), and improves maintainability. Now, function definition changes require modification in only one place, reducing the likelihood of regressions across platforms.

Example

To enable the override of a function foo in Silk:

  1. Add function declaration in main_overrides.h in the form of FOO_DECL(impl, ...) macro and function type definition with typedef FOO_DECL(t, *const);.
  2. Add the declaration to main.h:
    FOO_DECL(c);
    #ifndef foo
    #define foo(...) OVERRIDE_IMPL_SINGLE(foo, c, __VA_ARGS__)
    #endif
  3. Change the existing function definition to FOO_DECL(c) { ... }.
  4. Change all function references to include arch as the first argument.

Now, you can add arch-specific implementation (an example for x86 SSE):

  1. Add function override in main_sse.h:
    1. For single function override:
      #define foo(...) OVERRIDE_IMPL_SINGLE(foo, sse4_1, __VA_ARGS__)
    2. For RTCD override:
      extern OVERRIDE_IMPL_ARRAY_DECL(foo);
      #define foo(...) OVERRIDE_IMPL_ARRAY(foo, __VA_ARGS__)
  2. Add RTCD map definition in x86_silk_map.c:
    OVERRIDE_IMPL_ARRAY_DECL(foo) = {
      foo_c,                  /* non-sse */
      foo_c,
      foo_c,
      MAY_HAVE_SSE4_1( foo ), /* sse4.1 */
      MAY_HAVE_SSE4_1( foo )  /* avx */
    };
  3. Add arch-specific function definition in foo.c with
    FOO_DECL(sse4_1)
    {
        ...
    }

Decrease code repetition for arch-specific function overrides.

Signed-off-by: Marek Pikuła <m.pikula@partner.samsung.com>
@MarekPikula
Copy link
Author

Hi @jmvalin @rillian, could you take a look at this (or ping some other person who could make a comment)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant