Subject: Re: [PATCH] build: zig-build-system: Add CPU option

From: Efraim Flashner <[email-masked]>
Date: Sun, 3 Dec 2023 10:09:57 +0200

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: <[email-masked]>
> From: Ekaitz Zarraga <[email-masked]>
> 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 <[email-masked]>
> +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   <[email-masked]>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted