GNU bug report logs - #67075
[PATCH] build: zig-build-system: Add CPU option

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 67075 in the body.
You can then email your comments to 67075 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
To: "guix-patches <at> gnu.org" <guix-patches <at> gnu.org>
Subject: [PATCH] build: zig-build-system: Add CPU option
Date: Sat, 11 Nov 2023 13:09:07 +0000

From a647a8ee689022cafef4bab05784b32b1c97bee7 Mon Sep 17 00:00:00 2001
Message-ID: <a647a8ee689022cafef4bab05784b32b1c97bee7.1699708101.git.ekaitz <at> elenq.tech>
From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
Date: Sat, 11 Nov 2023 14:05:23 +0100
Subject: [PATCH] build: zig-build-system: Add CPU option

Zig packages are optimized by default, adding `-Dcpu=baseline` to the
build command builds them for an standard cpu that should work in every
machine.

This change sets that by default but also allows users to choose their
cpu by the `#:zig-cpu` argument.

* guix/build-system/zig.scm (build): add zig-cpu
* guix/build/zig-build-system.scm (zig-build) add zig-cpu

Change-Id: Ib4b2124179e7b5492e7c77c64e1f8336832032ea
---
 guix/build-system/zig.scm       | 2 ++
 guix/build/zig-build-system.scm | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/guix/build-system/zig.scm b/guix/build-system/zig.scm
index 16b8a712cc..f90e76104e 100644
--- a/guix/build-system/zig.scm
+++ b/guix/build-system/zig.scm
@@ -47,6 +47,7 @@ (define* (zig-build name inputs
                     source
                     (tests? #t)
                     (test-target #f)
+                    (zig-cpu #f)
                     (zig-build-flags ''())
                     (zig-test-flags ''())
                     (zig-release-type #f)
@@ -67,6 +68,7 @@ (define* (zig-build name inputs
                      #:source #+source
                      #:system #$system
                      #:test-target #$test-target
+                     #:zig-cpu #$zig-cpu
                      #:zig-build-flags #$zig-build-flags
                      #:zig-test-flags #$zig-test-flags
                      #:zig-release-type #$zig-release-type
diff --git a/guix/build/zig-build-system.scm b/guix/build/zig-build-system.scm
index d414ebfb17..99a81314d4 100644
--- a/guix/build/zig-build-system.scm
+++ b/guix/build/zig-build-system.scm
@@ -44,6 +44,7 @@ (define* (set-zig-global-cache-dir #:rest args)
   (setenv "ZIG_GLOBAL_CACHE_DIR" global-cache-dir))
 
 (define* (build #:key
+                zig-cpu
                 zig-build-flags
                 zig-release-type       ;; "safe", "fast" or "small" empty for a
                                        ;; debug build"
@@ -59,6 +60,7 @@ (define* (build #:key
                      ,@(if zig-release-type
                          (list (string-append "-Drelease-" zig-release-type))
                          '())
+                     ,(string-append "-Dcpu=" (or zig-cpu "baseline"))
                      ,@zig-build-flags)))
   (format #t "running: ~s~%" call)
   (apply invoke call)))

base-commit: af6105afc67a15a491a0a4fd18a28c9f801a0b94
-- 
2.41.0

Message #8 received at 67075 <at> debbugs.gnu.org (full text, mbox):

From: Efraim Flashner <efraim <at> flashner.co.il>
To: Ekaitz Zarraga <ekaitz <at> elenq.tech>
Cc: 67075 <at> debbugs.gnu.org
Subject: Re: [bug#67075] [PATCH] build: zig-build-system: Add CPU option
Date: Sun, 12 Nov 2023 16:38:58 +0200

[Message part 1 (text/plain, inline)]

What are the values that the compiler can take for this flag?  Also,
this seems like something that can be addressed with the tuning
mechanism, so we can run 'guix build foo --tune' and it'll do The Right
Thing™.

Alternatively, if we do go this route, you still need to update the
documentation.

On Sat, Nov 11, 2023 at 01:09:07PM +0000, Ekaitz Zarraga wrote:
> From a647a8ee689022cafef4bab05784b32b1c97bee7 Mon Sep 17 00:00:00 2001
> Message-ID: <a647a8ee689022cafef4bab05784b32b1c97bee7.1699708101.git.ekaitz <at> elenq.tech>
> From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
> Date: Sat, 11 Nov 2023 14:05:23 +0100
> Subject: [PATCH] build: zig-build-system: Add CPU option
> 
> Zig packages are optimized by default, adding `-Dcpu=baseline` to the
> build command builds them for an standard cpu that should work in every
> machine.
> 
> This change sets that by default but also allows users to choose their
> cpu by the `#:zig-cpu` argument.
> 
> * guix/build-system/zig.scm (build): add zig-cpu
> * guix/build/zig-build-system.scm (zig-build) add zig-cpu
> 
> Change-Id: Ib4b2124179e7b5492e7c77c64e1f8336832032ea
> ---
>  guix/build-system/zig.scm       | 2 ++
>  guix/build/zig-build-system.scm | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/guix/build-system/zig.scm b/guix/build-system/zig.scm
> index 16b8a712cc..f90e76104e 100644
> --- a/guix/build-system/zig.scm
> +++ b/guix/build-system/zig.scm
> @@ -47,6 +47,7 @@ (define* (zig-build name inputs
>                      source
>                      (tests? #t)
>                      (test-target #f)
> +                    (zig-cpu #f)
>                      (zig-build-flags ''())
>                      (zig-test-flags ''())
>                      (zig-release-type #f)
> @@ -67,6 +68,7 @@ (define* (zig-build name inputs
>                       #:source #+source
>                       #:system #$system
>                       #:test-target #$test-target
> +                     #:zig-cpu #$zig-cpu
>                       #:zig-build-flags #$zig-build-flags
>                       #:zig-test-flags #$zig-test-flags
>                       #:zig-release-type #$zig-release-type
> diff --git a/guix/build/zig-build-system.scm b/guix/build/zig-build-system.scm
> index d414ebfb17..99a81314d4 100644
> --- a/guix/build/zig-build-system.scm
> +++ b/guix/build/zig-build-system.scm
> @@ -44,6 +44,7 @@ (define* (set-zig-global-cache-dir #:rest args)
>    (setenv "ZIG_GLOBAL_CACHE_DIR" global-cache-dir))
>  
>  (define* (build #:key
> +                zig-cpu
>                  zig-build-flags
>                  zig-release-type       ;; "safe", "fast" or "small" empty for a
>                                         ;; debug build"
> @@ -59,6 +60,7 @@ (define* (build #:key
>                       ,@(if zig-release-type
>                           (list (string-append "-Drelease-" zig-release-type))
>                           '())
> +                     ,(string-append "-Dcpu=" (or zig-cpu "baseline"))
>                       ,@zig-build-flags)))
>    (format #t "running: ~s~%" call)
>    (apply invoke call)))
> 
> base-commit: af6105afc67a15a491a0a4fd18a28c9f801a0b94
> -- 
> 2.41.0
> 
> 
> 
> 
> 

-- 
Efraim Flashner   <efraim <at> flashner.co.il>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 67075 <at> debbugs.gnu.org (full text, mbox):

From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
To: Efraim Flashner <efraim <at> flashner.co.il>
Cc: 67075 <at> debbugs.gnu.org
Subject: Re: [bug#67075] [PATCH] build: zig-build-system: Add CPU option
Date: Sun, 12 Nov 2023 16:38:10 +0000

Hi Efraim,

On Sunday, November 12th, 2023 at 15:38, Efraim Flashner <efraim <at> flashner.co.il> wrote:

> What are the values that the compiler can take for this flag? Also,
> this seems like something that can be addressed with the tuning
> mechanism, so we can run 'guix build foo --tune' and it'll do The Right
> Thing™.

It's not that simple. I talked with Andrew Kelley and he told me the values
are obtained from LLVM and they have some patching. I checked the list and 
they are similar to those we use in the -march field:
https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html#x86-Options

The full list is accessible in `zig targets` and it's very extensive.

I don't know if we can just rely on that mechanism...

> Alternatively, if we do go this route, you still need to update the
> documentation.

That's very true. I will.

> On Sat, Nov 11, 2023 at 01:09:07PM +0000, Ekaitz Zarraga wrote:
> 
> > From a647a8ee689022cafef4bab05784b32b1c97bee7 Mon Sep 17 00:00:00 2001
> > Message-ID: a647a8ee689022cafef4bab05784b32b1c97bee7.1699708101.git.ekaitz <at> elenq.tech
> > From: Ekaitz Zarraga ekaitz <at> elenq.tech
> > Date: Sat, 11 Nov 2023 14:05:23 +0100
> > Subject: [PATCH] build: zig-build-system: Add CPU option
> > 
> > Zig packages are optimized by default, adding `-Dcpu=baseline` to the
> > build command builds them for an standard cpu that should work in every
> > machine.
> > 
> > This change sets that by default but also allows users to choose their
> > cpu by the `#:zig-cpu` argument.
> > 
> > * guix/build-system/zig.scm (build): add zig-cpu
> > * guix/build/zig-build-system.scm (zig-build) add zig-cpu
> > 
> > Change-Id: Ib4b2124179e7b5492e7c77c64e1f8336832032ea
> > ---
> > guix/build-system/zig.scm | 2 ++
> > guix/build/zig-build-system.scm | 2 ++
> > 2 files changed, 4 insertions(+)
> > 
> > diff --git a/guix/build-system/zig.scm b/guix/build-system/zig.scm
> > index 16b8a712cc..f90e76104e 100644
> > --- a/guix/build-system/zig.scm
> > +++ b/guix/build-system/zig.scm
> > @@ -47,6 +47,7 @@ (define* (zig-build name inputs
> > source
> > (tests? #t)
> > (test-target #f)
> > + (zig-cpu #f)
> > (zig-build-flags ''())
> > (zig-test-flags ''())
> > (zig-release-type #f)
> > @@ -67,6 +68,7 @@ (define* (zig-build name inputs
> > #:source #+source
> > #:system #$system
> > #:test-target #$test-target
> > + #:zig-cpu #$zig-cpu
> > #:zig-build-flags #$zig-build-flags
> > #:zig-test-flags #$zig-test-flags
> > #:zig-release-type #$zig-release-type
> > diff --git a/guix/build/zig-build-system.scm b/guix/build/zig-build-system.scm
> > index d414ebfb17..99a81314d4 100644
> > --- a/guix/build/zig-build-system.scm
> > +++ b/guix/build/zig-build-system.scm
> > @@ -44,6 +44,7 @@ (define* (set-zig-global-cache-dir #:rest args)
> > (setenv "ZIG_GLOBAL_CACHE_DIR" global-cache-dir))
> > 
> > (define* (build #:key
> > + zig-cpu
> > zig-build-flags
> > zig-release-type ;; "safe", "fast" or "small" empty for a
> > ;; debug build"
> > @@ -59,6 +60,7 @@ (define* (build #:key
> > ,@(if zig-release-type
> > (list (string-append "-Drelease-" zig-release-type))
> > '())
> > + ,(string-append "-Dcpu=" (or zig-cpu "baseline"))
> > ,@zig-build-flags)))
> > (format #t "running: ~s~%" call)
> > (apply invoke call)))
> > 
> > base-commit: af6105afc67a15a491a0a4fd18a28c9f801a0b94
> > --
> > 2.41.0
> 
> 
> --
> Efraim Flashner efraim <at> flashner.co.il רנשלפ םירפא
> 
> GPG key = A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351
> Confidentiality cannot be guaranteed on emails sent or received unencrypted

Message #14 received at 67075 <at> debbugs.gnu.org (full text, mbox):

From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
To: Efraim Flashner <efraim <at> flashner.co.il>
Cc: 67075 <at> debbugs.gnu.org
Subject: Re: [bug#67075] [PATCH] build: zig-build-system: Add CPU option
Date: Sun, 12 Nov 2023 16:41:07 +0000

Extra information I missed in the previous email:

> andrewrk: there is close to a 1-1 correspondence. zig uses underscores instead of dashes for naming these

So we can use a translation function from the tuning capabilites we already have and see how it goes.

Cheers

Message #17 received at 67075 <at> debbugs.gnu.org (full text, mbox):

From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
To: 67075 <at> debbugs.gnu.org
Subject: [PATCH] build: zig-build-system: Add CPU option
Date: Sat, 18 Nov 2023 17:52:48 +0100

[Message part 1 (text/plain, inline)]

Hi,

Following Efraim's suggestion I arranged a new patch around the tuning 
option. I needed to patch Zig itself to use baseline cpu by default, 
which is safer in general for us.

If anyone need a more specific cpu, can use the tuning option to choose it.

Maybe I should split the changes in Zig and the tuning system, but if I 
do they wont appear together, and they mostly only make sense if applied 
at once.

Please let me know if you like this one better.

Hope it's cool.

Cheers,
Ekaitz

[0001-gnu-zig-Make-compiler-tunable.patch (text/x-patch, attachment)]

Message #22 received at 67075-done <at> debbugs.gnu.org (full text, mbox):

From: Efraim Flashner <efraim <at> flashner.co.il>
To: Ekaitz Zarraga <ekaitz <at> elenq.tech>
Cc: 67075-done <at> debbugs.gnu.org
Subject: Re: [PATCH] build: zig-build-system: Add CPU option
Date: Sun, 3 Dec 2023 10:09:57 +0200

[Message part 1 (text/plain, inline)]

Patch applied with a couple of helper patches. Some comments inline.

On Sat, Nov 18, 2023 at 05:51:09PM +0100, Ekaitz Zarraga wrote:
> Hi,
> 
> Following Efraim's suggestion I arranged a new patch around the tuning
> option. I needed to patch Zig itself to use baseline cpu by default, which
> is safer in general for us.
> 
> If anyone need a more specific cpu, can use the tuning option to choose it.
> 
> Maybe I should split the changes in Zig and the tuning system, but if I do
> they wont appear together, and they mostly only make sense if applied at
> once.

I split the patch and applied the one adjusting zig to use
-Dcpu=baseline first.

> Please let me know if you like this one better.
> 
> Hope it's cool.
> 
> Cheers,
> Ekaitz

> From 03616b9764278e05d74f8e8bf5d65b043603a437 Mon Sep 17 00:00:00 2001
> Message-ID: <03616b9764278e05d74f8e8bf5d65b043603a437.1700325640.git.ekaitz <at> elenq.tech>
> From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
> Date: Sat, 18 Nov 2023 17:29:07 +0100
> Subject: [PATCH] gnu: zig: Make compiler tunable.
> 
> In order to make Zig tunable, we have to make sure by default it does
> the right thing, that is use the `baseline` CPU instead of the `native`
> one because the tuning system only adds an extra flag to the compiler
> command line execution.
> 
> * gnu/packages/patches/zig-use-baseline-cpu-by-default.patch: Add file.
> * gnu/packages/zig.scm(zig-0.10.1): Apply patch above.
> * guix/transformations.scm(tuning-compiler): Add support for zig.

Missing a change for gnu/local.mk

> Change-Id: I40bd28071c97c0dd0a907c704072b52b26d2de28
> ---
>  .../zig-use-baseline-cpu-by-default.patch     | 36 +++++++++++++++++++
>  gnu/packages/zig.scm                          |  3 +-
>  guix/transformations.scm                      | 22 ++++++++----
>  3 files changed, 54 insertions(+), 7 deletions(-)
>  create mode 100644 gnu/packages/patches/zig-use-baseline-cpu-by-default.patch
> 
> diff --git a/gnu/packages/patches/zig-use-baseline-cpu-by-default.patch b/gnu/packages/patches/zig-use-baseline-cpu-by-default.patch
> new file mode 100644
> index 0000000000..be78d9c6c7
> --- /dev/null
> +++ b/gnu/packages/patches/zig-use-baseline-cpu-by-default.patch
> @@ -0,0 +1,36 @@
> +From 1dc188129950031243c5a0c80ec2562fab8ec549 Mon Sep 17 00:00:00 2001
> +From: Ekaitz Zarraga <ekaitz <at> elenq.tech>
> +Date: Sat, 18 Nov 2023 15:04:16 +0100
> +Subject: [PATCH] Use `baseline` cpu by default.
> +
> +This helps Guix tune the package later. Tunning will only add
> +`-Dcpu=whatever` which should override the standard behaviour.
> +
> +Zig by default uses `native`, which interferes with our build process.
> +In our previous zig-build-system we chose to add `-Dcpu=baseline` flag
> +in each `zig build` execution, but that doesn't allow us to tune the
> +package later. Tunning is only designed to add extra flags in the
> +command line call, and we already had one set for the baseline case.
> +With this patch we set the standard behavior to `baseline` so we don't
> +need to add the `-Dcpu=baseline` flag in the zig-build-system and we can
> +tune with no issues.
> +---
> + lib/std/zig/CrossTarget.zig | 2 +-
> + 1 file changed, 1 insertion(+), 1 deletion(-)
> +
> +diff --git a/lib/std/zig/CrossTarget.zig b/lib/std/zig/CrossTarget.zig
> +index 6c59a4a3a..f5ec065fe 100644
> +--- a/lib/std/zig/CrossTarget.zig
> ++++ b/lib/std/zig/CrossTarget.zig
> +@@ -12,7 +12,7 @@ const mem = std.mem;
> + /// `null` means native.
> + cpu_arch: ?Target.Cpu.Arch = null,
> + 
> +-cpu_model: CpuModel = CpuModel.determined_by_cpu_arch,
> ++cpu_model: CpuModel = CpuModel.baseline,
> + 
> + /// Sparse set of CPU features to add to the set from `cpu_model`.
> + cpu_features_add: Target.Cpu.Feature.Set = Target.Cpu.Feature.Set.empty,
> +-- 
> +2.41.0
> +
> diff --git a/gnu/packages/zig.scm b/gnu/packages/zig.scm
> index dcca9a1121..82b83f44c2 100644
> --- a/gnu/packages/zig.scm
> +++ b/gnu/packages/zig.scm
> @@ -42,7 +42,8 @@ (define-public zig-0.10
>         (file-name (git-file-name name version))
>         (sha256
>          (base32 "1sh5xjsksl52i4cfv1qj36sz5h0ln7cq4pdhgs3960mk8a90im7b"))
> -       (patches (search-patches "zig-do-not-link-against-librt.patch"))))
> +       (patches (search-patches "zig-do-not-link-against-librt.patch"
> +                                "zig-use-Dcpu-baseline-by-default.patch"))))
>      (build-system cmake-build-system)
>      (inputs
>       (list clang-15 ; Clang propagates llvm.
> diff --git a/guix/transformations.scm b/guix/transformations.scm
> index 9cba6bedab..8964feb046 100644
> --- a/guix/transformations.scm
> +++ b/guix/transformations.scm
> @@ -439,7 +439,8 @@ (define tuning-compiler
>  actual compiler."
>      (define wrapper
>        #~(begin
> -          (use-modules (ice-9 match))
> +          (use-modules (ice-9 match)
> +                       (ice-9 string-fun))
>  
>            (define psabi #$(gcc-architecture->micro-architecture-level
>                              micro-architecture))
> @@ -486,11 +487,20 @@ (define tuning-compiler
>                  (apply
>                    execl next
>                         (append (cons next arguments)
> -                         (if (and (search-next "go")
> +                         (cond
> +                           ((and (search-next "go")
>                                    (string=? next (search-next "go")))
> -                           '()
> -                           (list (string-append "-march="
> -                                                #$micro-architecture)))))))))))
> +                           '())
> +                           ((and (search-next "zig")
> +                                  (string=? next (search-next "zig"))

Indentation is off, the and should end with string=?

> +                            (list (string-append

This I changed to `(,(string-append, otherwise it complains about
receiving a list instead of a string.

> +                                    ;; https://issues.guix.gnu.org/67075#3
> +                                    "-Dcpu="
> +                                    (string-replace-substring
> +                                      #$micro-architecture "-" "_")))))
> +                           (else
> +                             (list (string-append "-march="
> +                                     #$micro-architecture))))))))))))
>  
>      (define program
>        (program-file (string-append "tuning-compiler-wrapper-" micro-architecture)
> @@ -508,7 +518,7 @@ (define tuning-compiler
>                                       (symlink #$program
>                                                (string-append bin "/" program)))
>                                     '("cc" "gcc" "clang" "g++" "c++" "clang++"
> -                                     "go")))))))
> +                                     "go" "zig")))))))
>  
>  (define (build-system-with-tuning-compiler bs micro-architecture)
>    "Return a variant of BS, a build system, that ensures that the compiler that
> 
> base-commit: a514973413dc8179100ef8f0fcf41f5ac38c982f
> -- 
> 2.41.0
> 


-- 
Efraim Flashner   <efraim <at> flashner.co.il>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.