diff mbox series

[v4,04/13] package/gemmlowp: new package

Message ID 20230127141058.2180747-4-james.hilliard1@gmail.com
State Superseded, archived
Headers show
Series [v4,01/13] package/flatbuffers: build position independent code | expand

Commit Message

James Hilliard Jan. 27, 2023, 2:10 p.m. UTC 
From: Stefan Hager <stefan.hager@ginzinger.com>

This package is required by tensorflow-lite.

Tested-by: Stefan Hager <stefan.hager@ginzinger.com>
Signed-off-by: Stefan Hager <stefan.hager@ginzinger.com>
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes v2 -> v3:
  - add C++11 dependency
---
 DEVELOPERS                     |  1 +
 package/Config.in              |  1 +
 package/gemmlowp/Config.in     | 12 ++++++++++++
 package/gemmlowp/gemmlowp.hash |  4 ++++
 package/gemmlowp/gemmlowp.mk   | 15 +++++++++++++++
 5 files changed, 33 insertions(+)
 create mode 100644 package/gemmlowp/Config.in
 create mode 100644 package/gemmlowp/gemmlowp.hash
 create mode 100644 package/gemmlowp/gemmlowp.mk

Comments

Yann E. MORIN Jan. 29, 2023, 9:45 p.m. UTC | #1 
James, All,

On 2023-01-27 07:10 -0700, James Hilliard spake thusly:
> From: Stefan Hager <stefan.hager@ginzinger.com>
> 
> This package is required by tensorflow-lite.
> 
> Tested-by: Stefan Hager <stefan.hager@ginzinger.com>
> Signed-off-by: Stefan Hager <stefan.hager@ginzinger.com>
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
[--SNIP--]
> diff --git a/package/gemmlowp/gemmlowp.mk b/package/gemmlowp/gemmlowp.mk
> new file mode 100644
> index 0000000000..1a36fc48f7
> --- /dev/null
> +++ b/package/gemmlowp/gemmlowp.mk
> @@ -0,0 +1,15 @@
> +################################################################################
> +#
> +# gemmlowp
> +#
> +################################################################################
> +
> +GEMMLOWP_VERSION = 08e4bb339e34017a0835269d4a37c4ea04d15a69
> +GEMMLOWP_SITE = $(call github,google,gemmlowp,$(GEMMLOWP_VERSION))
> +GEMMLOWP_LICENSE = Apache-2.0
> +GEMMLOWP_LICENSE_FILES = LICENSE
> +GEMMLOWP_INSTALL_STAGING = YES
> +GEMMLOWP_INSTALL_TARGET = NO
> +GEMMLOWP_SUBDIR = contrib

Upstream is very careful to point to some optimisations that are
required or performance will suffer.

Notably, should be enabled:
  - for x86: sse4.1
  - for ARM: NEON

For NEON, it seems this is automatically handled, though, based on
looking at some macros:

    internal/detect_platform.h
    85 #if (defined __ARM_NEON) || (defined __ARM_NEON__)
    86 #define GEMMLOWP_NEON
    87 #endif

For x86, we should probably have code like (what we have as
BR2_X86_CPU_HAS_SSE4 seems to be SSE4.1, but it is not clear whether
there is an actual delta between SSE4 and SSE4.1 [0], so let's be safe
and assume that BR2_X86_CPU_HAS_SSE4 is not enough):

    ifeq ($(BR2_X86_CPU_HAS_SSE42),y)
    GEMMLOWP_OPTIM += -msse4.1
    endif

However, that same header seems to also have some detection based on
macros... This needs to be cleared, maybe?

[0] https://en.wikipedia.org/wiki/SSE4

Regards,
Yann E. MORIN.

> +$(eval $(cmake-package))
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
Yann E. MORIN Jan. 29, 2023, 9:46 p.m. UTC | #2 
James, All,

On 2023-01-27 07:10 -0700, James Hilliard spake thusly:
> From: Stefan Hager <stefan.hager@ginzinger.com>
> 
> This package is required by tensorflow-lite.
> 
> Tested-by: Stefan Hager <stefan.hager@ginzinger.com>
> Signed-off-by: Stefan Hager <stefan.hager@ginzinger.com>
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
[--SNIP--]
> diff --git a/package/gemmlowp/gemmlowp.mk b/package/gemmlowp/gemmlowp.mk
> new file mode 100644
> index 0000000000..1a36fc48f7
> --- /dev/null
> +++ b/package/gemmlowp/gemmlowp.mk
> @@ -0,0 +1,15 @@
> +################################################################################
> +#
> +# gemmlowp
> +#
> +################################################################################
> +
> +GEMMLOWP_VERSION = 08e4bb339e34017a0835269d4a37c4ea04d15a69
> +GEMMLOWP_SITE = $(call github,google,gemmlowp,$(GEMMLOWP_VERSION))
> +GEMMLOWP_LICENSE = Apache-2.0
> +GEMMLOWP_LICENSE_FILES = LICENSE
> +GEMMLOWP_INSTALL_STAGING = YES
> +GEMMLOWP_INSTALL_TARGET = NO

Why not install to target? This needs a little explanations, even if
it's only a comment above, like:  # Only installs headers

Regards,
Yann E. MORIN.

> +GEMMLOWP_SUBDIR = contrib
> +
> +$(eval $(cmake-package))
> -- 
> 2.34.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
James Hilliard Jan. 29, 2023, 11:51 p.m. UTC | #3 
On Sun, Jan 29, 2023 at 2:46 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2023-01-27 07:10 -0700, James Hilliard spake thusly:
> > From: Stefan Hager <stefan.hager@ginzinger.com>
> >
> > This package is required by tensorflow-lite.
> >
> > Tested-by: Stefan Hager <stefan.hager@ginzinger.com>
> > Signed-off-by: Stefan Hager <stefan.hager@ginzinger.com>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> [--SNIP--]
> > diff --git a/package/gemmlowp/gemmlowp.mk b/package/gemmlowp/gemmlowp.mk
> > new file mode 100644
> > index 0000000000..1a36fc48f7
> > --- /dev/null
> > +++ b/package/gemmlowp/gemmlowp.mk
> > @@ -0,0 +1,15 @@
> > +################################################################################
> > +#
> > +# gemmlowp
> > +#
> > +################################################################################
> > +
> > +GEMMLOWP_VERSION = 08e4bb339e34017a0835269d4a37c4ea04d15a69
> > +GEMMLOWP_SITE = $(call github,google,gemmlowp,$(GEMMLOWP_VERSION))
> > +GEMMLOWP_LICENSE = Apache-2.0
> > +GEMMLOWP_LICENSE_FILES = LICENSE
> > +GEMMLOWP_INSTALL_STAGING = YES
> > +GEMMLOWP_INSTALL_TARGET = NO
>
> Why not install to target? This needs a little explanations, even if
> it's only a comment above, like:  # Only installs headers

Yeah, it's another header-only library.

>
> Regards,
> Yann E. MORIN.
>
> > +GEMMLOWP_SUBDIR = contrib
> > +
> > +$(eval $(cmake-package))
> > --
> > 2.34.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
James Hilliard Jan. 30, 2023, 3:17 a.m. UTC | #4 
On Sun, Jan 29, 2023 at 2:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2023-01-27 07:10 -0700, James Hilliard spake thusly:
> > From: Stefan Hager <stefan.hager@ginzinger.com>
> >
> > This package is required by tensorflow-lite.
> >
> > Tested-by: Stefan Hager <stefan.hager@ginzinger.com>
> > Signed-off-by: Stefan Hager <stefan.hager@ginzinger.com>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> [--SNIP--]
> > diff --git a/package/gemmlowp/gemmlowp.mk b/package/gemmlowp/gemmlowp.mk
> > new file mode 100644
> > index 0000000000..1a36fc48f7
> > --- /dev/null
> > +++ b/package/gemmlowp/gemmlowp.mk
> > @@ -0,0 +1,15 @@
> > +################################################################################
> > +#
> > +# gemmlowp
> > +#
> > +################################################################################
> > +
> > +GEMMLOWP_VERSION = 08e4bb339e34017a0835269d4a37c4ea04d15a69
> > +GEMMLOWP_SITE = $(call github,google,gemmlowp,$(GEMMLOWP_VERSION))
> > +GEMMLOWP_LICENSE = Apache-2.0
> > +GEMMLOWP_LICENSE_FILES = LICENSE
> > +GEMMLOWP_INSTALL_STAGING = YES
> > +GEMMLOWP_INSTALL_TARGET = NO
> > +GEMMLOWP_SUBDIR = contrib
>
> Upstream is very careful to point to some optimisations that are
> required or performance will suffer.
>
> Notably, should be enabled:
>   - for x86: sse4.1
>   - for ARM: NEON
>
> For NEON, it seems this is automatically handled, though, based on
> looking at some macros:
>
>     internal/detect_platform.h
>     85 #if (defined __ARM_NEON) || (defined __ARM_NEON__)
>     86 #define GEMMLOWP_NEON
>     87 #endif
>
> For x86, we should probably have code like (what we have as
> BR2_X86_CPU_HAS_SSE4 seems to be SSE4.1, but it is not clear whether
> there is an actual delta between SSE4 and SSE4.1 [0], so let's be safe
> and assume that BR2_X86_CPU_HAS_SSE4 is not enough):
>
>     ifeq ($(BR2_X86_CPU_HAS_SSE42),y)
>     GEMMLOWP_OPTIM += -msse4.1
>     endif
>
> However, that same header seems to also have some detection based on
> macros... This needs to be cleared, maybe?

Well gemmlowp is effectively a header only library so optimization flags would
not be set for the gemmlowp package itself but rather in something like the
tensorflow-lite package which consumes the headers AFAIU.

Note there's technically an unused deprecated library that gets installed
to staging, we don't install it to target since we don't have anything consuming
that deprecated interface at all:
https://github.com/google/gemmlowp#old-eightbitintgemm-legacy-deprecated-interface

>
> [0] https://en.wikipedia.org/wiki/SSE4
>
> Regards,
> Yann E. MORIN.
>
> > +$(eval $(cmake-package))
> > --
> > 2.34.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
Yann E. MORIN Jan. 30, 2023, 7:13 a.m. UTC | #5 
James, All,

On 2023-01-29 20:17 -0700, James Hilliard spake thusly:
> On Sun, Jan 29, 2023 at 2:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
[--SNIP--]
> > For x86, we should probably have code like (what we have as
> > BR2_X86_CPU_HAS_SSE4 seems to be SSE4.1, but it is not clear whether
> > there is an actual delta between SSE4 and SSE4.1 [0], so let's be safe
> > and assume that BR2_X86_CPU_HAS_SSE4 is not enough):
> >
> >     ifeq ($(BR2_X86_CPU_HAS_SSE42),y)
> >     GEMMLOWP_OPTIM += -msse4.1
> >     endif
> >
> > However, that same header seems to also have some detection based on
> > macros... This needs to be cleared, maybe?
> 
> Well gemmlowp is effectively a header only library so optimization flags would
> not be set for the gemmlowp package itself but rather in something like the
> tensorflow-lite package which consumes the headers AFAIU.

Yeah, I noticed that it was a header-only library only later...

Still, for those packages, gemmlowp, neon-2-sse, maybe fxdiv, they do
have a requirement that SSE be needed, so they should depend on that.
For example:

    config BR2_PACKAGES_GEMMLOWP_ARCH_SUPPORTS
        bool
        default y if BR2_X86_CPU_HAS_SSE42
        default y if BR2_ARM_CPU_HAS_NEON

Or:

    config BR2_PACKAGES_NEON_2_SEE_ARCH_SUPPORTS
        bool
        default y if BR2_X86_CPU_HAS_SSE3

Otherwise, those packages (notable neon-2-sse) don't make much sense, or
will be horribly slow...

Can you send a set of patches, please?

Regards,
Yann E. MORIN.

> Note there's technically an unused deprecated library that gets installed
> to staging, we don't install it to target since we don't have anything consuming
> that deprecated interface at all:
> https://github.com/google/gemmlowp#old-eightbitintgemm-legacy-deprecated-interface
> 
> >
> > [0] https://en.wikipedia.org/wiki/SSE4
> >
> > Regards,
> > Yann E. MORIN.
> >
> > > +$(eval $(cmake-package))
> > > --
> > > 2.34.1
> > >
> > > _______________________________________________
> > > buildroot mailing list
> > > buildroot@buildroot.org
> > > https://lists.buildroot.org/mailman/listinfo/buildroot
> >
> > --
> > .-----------------.--------------------.------------------.--------------------.
> > |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> > | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> > | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> > '------------------------------^-------^------------------^--------------------'
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
James Hilliard Jan. 30, 2023, 7:45 a.m. UTC | #6 
On Mon, Jan 30, 2023 at 12:13 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> James, All,
>
> On 2023-01-29 20:17 -0700, James Hilliard spake thusly:
> > On Sun, Jan 29, 2023 at 2:45 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> [--SNIP--]
> > > For x86, we should probably have code like (what we have as
> > > BR2_X86_CPU_HAS_SSE4 seems to be SSE4.1, but it is not clear whether
> > > there is an actual delta between SSE4 and SSE4.1 [0], so let's be safe
> > > and assume that BR2_X86_CPU_HAS_SSE4 is not enough):
> > >
> > >     ifeq ($(BR2_X86_CPU_HAS_SSE42),y)
> > >     GEMMLOWP_OPTIM += -msse4.1
> > >     endif
> > >
> > > However, that same header seems to also have some detection based on
> > > macros... This needs to be cleared, maybe?
> >
> > Well gemmlowp is effectively a header only library so optimization flags would
> > not be set for the gemmlowp package itself but rather in something like the
> > tensorflow-lite package which consumes the headers AFAIU.
>
> Yeah, I noticed that it was a header-only library only later...
>
> Still, for those packages, gemmlowp, neon-2-sse, maybe fxdiv, they do
> have a requirement that SSE be needed, so they should depend on that.
> For example:
>
>     config BR2_PACKAGES_GEMMLOWP_ARCH_SUPPORTS
>         bool
>         default y if BR2_X86_CPU_HAS_SSE42
>         default y if BR2_ARM_CPU_HAS_NEON
>
> Or:
>
>     config BR2_PACKAGES_NEON_2_SEE_ARCH_SUPPORTS
>         bool
>         default y if BR2_X86_CPU_HAS_SSE3
>
> Otherwise, those packages (notable neon-2-sse) don't make much sense, or
> will be horribly slow...

The tensorflow-lite build system enforces that these header libraries are
present even in cases where they are not used, for example neon-2-sse
isn't always used but tensorflow-lite always enforces that it is present even
on architectures where it is not required/used.

Changing tensorflow-lite to not enforce the presence of these libraries
would be rather complex as they tend to be used via conditional imports
with the presence checks being done in cmake.

Because of this we can't restrict the selection of these header-only libraries
to their compatible architectures without breaking tensorflow-lite.

>
> Can you send a set of patches, please?
>
> Regards,
> Yann E. MORIN.
>
> > Note there's technically an unused deprecated library that gets installed
> > to staging, we don't install it to target since we don't have anything consuming
> > that deprecated interface at all:
> > https://github.com/google/gemmlowp#old-eightbitintgemm-legacy-deprecated-interface
> >
> > >
> > > [0] https://en.wikipedia.org/wiki/SSE4
> > >
> > > Regards,
> > > Yann E. MORIN.
> > >
> > > > +$(eval $(cmake-package))
> > > > --
> > > > 2.34.1
> > > >
> > > > _______________________________________________
> > > > buildroot mailing list
> > > > buildroot@buildroot.org
> > > > https://lists.buildroot.org/mailman/listinfo/buildroot
> > >
> > > --
> > > .-----------------.--------------------.------------------.--------------------.
> > > |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> > > | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> > > | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> > > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> > > '------------------------------^-------^------------------^--------------------'
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
diff mbox series

Patch

diff --git a/DEVELOPERS b/DEVELOPERS
index ba26d5f999..f48ea66660 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -2703,6 +2703,7 @@  F:	package/ti-gfx/
 
 N:	Stefan Hager <stefan.hager@ginzinger.com>
 F:	package/cpuinfo/
+F:	package/gemmlowp/
 F:	package/ruy/
 
 N:	Stefan Ott <stefan@ott.net>
diff --git a/package/Config.in b/package/Config.in
index 29d55ef2b3..8f78080963 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2009,6 +2009,7 @@  menu "Other"
 	source "package/flatcc/Config.in"
 	source "package/gconf/Config.in"
 	source "package/gdal/Config.in"
+	source "package/gemmlowp/Config.in"
 	source "package/gflags/Config.in"
 	source "package/gli/Config.in"
 	source "package/glibmm/Config.in"
diff --git a/package/gemmlowp/Config.in b/package/gemmlowp/Config.in
new file mode 100644
index 0000000000..379450d1b0
--- /dev/null
+++ b/package/gemmlowp/Config.in
@@ -0,0 +1,12 @@ 
+config BR2_PACKAGE_GEMMLOWP
+	bool "gemmlowp"
+	depends on BR2_INSTALL_LIBSTDCPP
+	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8 # C++11
+	help
+	  Low-precision matrix multiplication.
+
+	  https://github.com/google/gemmlowp
+
+comment "gemmlowp needs a toolchain w/ C++11"
+	depends on !BR2_INSTALL_LIBSTDCPP || \
+		!BR2_TOOLCHAIN_GCC_AT_LEAST_4_8
diff --git a/package/gemmlowp/gemmlowp.hash b/package/gemmlowp/gemmlowp.hash
new file mode 100644
index 0000000000..7c56a18bb6
--- /dev/null
+++ b/package/gemmlowp/gemmlowp.hash
@@ -0,0 +1,4 @@ 
+# Locally calculated
+sha256  cc8a22b6f071c3781e6b4b72654c89b1cdc198e72ebadebb17638eac205344c1  gemmlowp-08e4bb339e34017a0835269d4a37c4ea04d15a69.tar.gz
+# License files, locally calculated
+sha256  cfc7749b96f63bd31c3c42b5c471bf756814053e847c10f3eb003417bc523d30  LICENSE
diff --git a/package/gemmlowp/gemmlowp.mk b/package/gemmlowp/gemmlowp.mk
new file mode 100644
index 0000000000..1a36fc48f7
--- /dev/null
+++ b/package/gemmlowp/gemmlowp.mk
@@ -0,0 +1,15 @@ 
+################################################################################
+#
+# gemmlowp
+#
+################################################################################
+
+GEMMLOWP_VERSION = 08e4bb339e34017a0835269d4a37c4ea04d15a69
+GEMMLOWP_SITE = $(call github,google,gemmlowp,$(GEMMLOWP_VERSION))
+GEMMLOWP_LICENSE = Apache-2.0
+GEMMLOWP_LICENSE_FILES = LICENSE
+GEMMLOWP_INSTALL_STAGING = YES
+GEMMLOWP_INSTALL_TARGET = NO
+GEMMLOWP_SUBDIR = contrib
+
+$(eval $(cmake-package))