[Buildroot] [PATCH 1/1] package/riscv-coremark: Create package with CoreMark benchmark for RISC-V

[Buildroot] [PATCH 1/1] package/riscv-coremark: Create package with CoreMark benchmark for RISC-V

[Eryk Szpotanski via buildroot]

[-- Attachment #1.1: Type: text/plain, Size: 4634 bytes --]

From 3c09f5c971248c8c9c05a8bab79a5ddb45b40eb7 Mon Sep 17 00:00:00 2001
From: Eryk Szpotanski <eszpotanski@antmicro.com>
Date: Thu, 9 May 2024 12:34:08 +0200
Subject: [PATCH 1/1] package/riscv-coremark: Create package with CoreMark
 benchmark for RISC-V

Signed-off-by: Eryk Szpotanski <eszpotanski@antmicro.com>
---
 package/Config.in                             |  1 +
 ...ype-to-match-it-with-definition-from.patch | 27 +++++++++++++++++++
 package/riscv-coremark/Config.in              |  8 ++++++
 package/riscv-coremark/riscv-coremark.hash    |  3 +++
 package/riscv-coremark/riscv-coremark.mk      | 22 +++++++++++++++
 5 files changed, 61 insertions(+)
 create mode 100644
package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
 create mode 100644 package/riscv-coremark/Config.in
 create mode 100644 package/riscv-coremark/riscv-coremark.hash
 create mode 100644 package/riscv-coremark/riscv-coremark.mk

diff --git a/package/Config.in b/package/Config.in
index 2ac351cce5..09dab28ef3 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -145,6 +145,7 @@ menu "Debugging, profiling and benchmark"
  source "package/racehound/Config.in"
  source "package/ramsmp/Config.in"
  source "package/ramspeed/Config.in"
+ source "package/riscv-coremark/Config.in"
  source "package/rt-tests/Config.in"
  source "package/rwmem/Config.in"
  source "package/sentry-native/Config.in"
diff --git
a/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
b/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
new file mode 100644
index 0000000000..fda488198d
--- /dev/null
+++
b/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
@@ -0,0 +1,27 @@
+From a6c8e2e00e5534b340885181d99ecf19286a9ff8 Mon Sep 17 00:00:00 2001
+From: Eryk Szpotanski <eszpotanski@antmicro.com>
+Date: Tue, 6 Aug 2024 13:44:21 +0200
+Subject: [PATCH] Update clock_t type, to match it with external definition
+Upstream: N/A This patch adjust the clock_t type to match it with the
definition from RISC-V toolchain
+
+Signed-off-by: Eryk Szpotanski <eszpotanski@antmicro.com>
+---
+ riscv64/core_portme.h | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/riscv64/core_portme.h b/riscv64/core_portme.h
+index 4e28afd..1e7708e 100755
+--- a/riscv64/core_portme.h
++++ b/riscv64/core_portme.h
+@@ -71,7 +71,7 @@ typedef clock_t CORE_TICKS;
+      Note these need to match the size of the clock output and the xLen
the processor supports
+  */
+ typedef unsigned long int size_t;
+-typedef unsigned long int clock_t;
++typedef long int clock_t;
+ typedef clock_t CORE_TICKS;
+ #endif
+
+--
+2.45.2
+
diff --git a/package/riscv-coremark/Config.in
b/package/riscv-coremark/Config.in
new file mode 100644
index 0000000000..c9ed47befa
--- /dev/null
+++ b/package/riscv-coremark/Config.in
@@ -0,0 +1,8 @@
+config BR2_PACKAGE_RISCV_COREMARK
+ bool "riscv-coremark"
+ depends on BR2_RISCV_64
+ help
+  CoreMark benchmark with utility files provided to
+  support RISC-V architecture
+
+  https://github.com/riscv-boom/riscv-coremark/
diff --git a/package/riscv-coremark/riscv-coremark.hash
b/package/riscv-coremark/riscv-coremark.hash
new file mode 100644
index 0000000000..5be3db6bf8
--- /dev/null
+++ b/package/riscv-coremark/riscv-coremark.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256  0dde3285b9a1e6bb22bab82e485b5cf58a3e6a026d9cdc22102619a5584a6c7f
 riscv-coremark-6e1d72b864e45f67031ffaedb0b01b5d030d6d3c-br1.tar.gz
+sha256  fc8a88c0c1b5a6bc7358f18c43cdd562c1bfd83b08d81a43f726917582c0e260
 LICENSE
diff --git a/package/riscv-coremark/riscv-coremark.mk
b/package/riscv-coremark/riscv-coremark.mk
new file mode 100644
index 0000000000..819fac2313
--- /dev/null
+++ b/package/riscv-coremark/riscv-coremark.mk
@@ -0,0 +1,22 @@
+################################################################################
+#
+# RISC-V Coremark
+#
+################################################################################
+
+RISCV_COREMARK_SITE_METHOD = git
+RISCV_COREMARK_GIT_SUBMODULES = YES
+RISCV_COREMARK_VERSION = 6e1d72b864e45f67031ffaedb0b01b5d030d6d3c
+RISCV_COREMARK_SITE = https://github.com/riscv-boom/riscv-coremark.git
+RISCV_COREMARK_LICENSE = BSD-3
+RISCV_COREMARK_LICENSE_FILE = LICENSE
+
+define RISCV_COREMARK_BUILD_CMDS
+ $(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" -C $(@D)/coremark
PORT_DIR=$(@D)/riscv64 compile
+endef
+
+define RISCV_COREMARK_INSTALL_TARGET_CMDS
+ $(INSTALL) -D $(@D)/coremark/coremark.riscv
$(TARGET_DIR)/usr/bin/coremark.riscv
+endef
+
+$(eval $(generic-package))
-- 
2.45.2

[-- Attachment #1.2: Type: text/html, Size: 5980 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/riscv-coremark: Create package with CoreMark benchmark for RISC-V

[Thomas Petazzoni via buildroot]

Hello Eryk,

On Tue, 6 Aug 2024 18:39:58 +0200
Eryk Szpotanski <eszpotanski@antmicro.com> wrote:

> From 3c09f5c971248c8c9c05a8bab79a5ddb45b40eb7 Mon Sep 17 00:00:00 2001
> From: Eryk Szpotanski <eszpotanski@antmicro.com>
> Date: Thu, 9 May 2024 12:34:08 +0200
> Subject: [PATCH 1/1] package/riscv-coremark: Create package with CoreMark
>  benchmark for RISC-V
> 
> Signed-off-by: Eryk Szpotanski <eszpotanski@antmicro.com>

Thanks a lot for your patch! However, it looks like you haven't used
"git send-email", so your patch is badly line-wrapped and cannot be
applied. Could you use "git send-email" to send a new iteration?

See a few quick review comments below.

First, why do we need a riscv-coremark as opposed to an
architecture-agnostic coremark?

> ---
>  package/Config.in                             |  1 +
>  ...ype-to-match-it-with-definition-from.patch | 27 +++++++++++++++++++
>  package/riscv-coremark/Config.in              |  8 ++++++
>  package/riscv-coremark/riscv-coremark.hash    |  3 +++
>  package/riscv-coremark/riscv-coremark.mk      | 22 +++++++++++++++
>  5 files changed, 61 insertions(+)

Please add yourself to the DEVELOPERS as the maintainer for this
package.


> b/package/riscv-coremark/0001-Update-clock_t-type-to-match-it-with-definition-from.patch
> @@ -0,0 +1,27 @@
> +From a6c8e2e00e5534b340885181d99ecf19286a9ff8 Mon Sep 17 00:00:00 2001
> +From: Eryk Szpotanski <eszpotanski@antmicro.com>
> +Date: Tue, 6 Aug 2024 13:44:21 +0200
> +Subject: [PATCH] Update clock_t type, to match it with external definition
> +Upstream: N/A This patch adjust the clock_t type to match it with the
> definition from RISC-V toolchain

Why is that not applicable upstream? Especially if upstream is anyway
RISC-V specific?


> diff --git a/package/riscv-coremark/Config.in
> b/package/riscv-coremark/Config.in
> new file mode 100644
> index 0000000000..c9ed47befa
> --- /dev/null
> +++ b/package/riscv-coremark/Config.in
> @@ -0,0 +1,8 @@
> +config BR2_PACKAGE_RISCV_COREMARK
> + bool "riscv-coremark"
> + depends on BR2_RISCV_64

As asked above: why is this RISC-V specific?


> diff --git a/package/riscv-coremark/riscv-coremark.mk
> b/package/riscv-coremark/riscv-coremark.mk
> new file mode 100644
> index 0000000000..819fac2313
> --- /dev/null
> +++ b/package/riscv-coremark/riscv-coremark.mk
> @@ -0,0 +1,22 @@
> +################################################################################
> +#
> +# RISC-V Coremark
> +#
> +################################################################################
> +
> +RISCV_COREMARK_SITE_METHOD = git
> +RISCV_COREMARK_GIT_SUBMODULES = YES
> +RISCV_COREMARK_VERSION = 6e1d72b864e45f67031ffaedb0b01b5d030d6d3c
> +RISCV_COREMARK_SITE = https://github.com/riscv-boom/riscv-coremark.git
> +RISCV_COREMARK_LICENSE = BSD-3

Should be BSD-3-Clause

> +RISCV_COREMARK_LICENSE_FILE = LICENSE
> +
> +define RISCV_COREMARK_BUILD_CMDS
> + $(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" -C $(@D)/coremark
> PORT_DIR=$(@D)/riscv64 compile

Could you try to use $(TARGET_CONFIGURE_OPTS) instead of just CC="$(TARGET_CC)" ?

> +endef
> +
> +define RISCV_COREMARK_INSTALL_TARGET_CMDS
> + $(INSTALL) -D $(@D)/coremark/coremark.riscv
> $(TARGET_DIR)/usr/bin/coremark.riscv

Please add -m 0755 to this $(INSTALL) invocation.

Overall, it looks reasonable, really the main concern is why do we need
a RISC-V specific version of this? What if we want coremark on other
architectures?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot