[bug#64188] [PATCH 0/8] More package tuning

[Efraim Flashner <efraim@flashner.co.il>]

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

On Mon, Jul 17, 2023 at 05:41:35PM +0200, Ludovic Courtès wrote:
> Hi,
> 
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > On Thu, Jul 13, 2023 at 05:27:21PM +0200, Ludovic Courtès wrote:
> 
> [...]
> 
> >> It looks like we’re now adding the ‘set-microarchitecture’ phase
> >> unconditionally, not just for go.  For example:
> >> 
> >> --8<---------------cut here---------------start------------->8---
> >> $ ./pre-inst-env guix build --tune eigen-benchmarks --log-file
> >> guix build: tuning eigen-benchmarks@3.4.0 for CPU skylake
> >> https://ci.guix.gnu.org/log/djwka1jhzhk08yb23as83yk5hysn0pky-eigen-benchmarks-3.4.0
> >> $ wget -qO- https://ci.guix.gnu.org/log/djwka1jhzhk08yb23as83yk5hysn0pky-eigen-benchmarks-3.4.0 |gunzip -c| grep -C3 set-micro
> >> phase `reset-gzip-timestamps' succeeded after 0.0 seconds
> >> starting phase `compress-documentation'
> >> phase `compress-documentation' succeeded after 0.0 seconds
> >> starting phase `set-microarchitecture'
> >> Setting GOAMD to "v3".
> >> phase `set-microarchitecture' succeeded after 0.0 seconds
> >> @ build-succeeded /gnu/store/pdz0g9q2yd9i1jkbhk2rnbfa88ngvffw-eigen-benchmarks-3.4.0.drv -
> >> --8<---------------cut here---------------end--------------->8---
> >> 
> >> What I had in mind was to have a procedure similar to ‘tuning-compiler’
> >> that would return a wrapper around the “go” binary that would set
> >> ‘GOAMD’ (or similar).  That way the change would be well isolated.
> >> 
> >> Could you look into providing a patch for that?
> >> 
> >> Thanks in advance!
> >> 
> >> Ludo’.
> >
> > That's actually really surprising to me. I thought that if you tried to
> > add a phase after a non-existent phase then it just wouldn't be added.
> 
> Actually I thought so too.  :-)
> 
> But anyway, the point is that we’re modifying phases unconditionally
> (whether or not this has an effect), and it would be nicer to avoid it
> IMO.
> 
> > I tried just wrapping the call to the 'go' binary itself so that every
> > time 'go' was called it would also set the environment variable setting
> > the optimization level but I was having a hard time working that. While
> > experimenting I did change what I had written to check for the
> > 'setup-go-environment phase, and if it existed to add the optimization
> > at the end of that phase.
> >
> > I have the part with wrapping the go binary as a WIP, and when it's
> > ready I'll post both parts so we can choose which one seems better. I
> > like the idea of go being wrapped, it makes it easier to just add in the
> > optimizations whenever go is added to a package. On the other hand I
> > like the extra phase, since it's already done :)
> 
> Not a valid argument! :-)  We can discuss the implementation on IRC if
> you want.  It might be that we can slightly generalize ‘tuning-compiler’
> so that it works for go (perhaps there’s an option like ‘-march’ that we
> could use instead of setting ‘GOAMD’?).

I found -goarch, but it's for cross-compiling and wouldn't take
x86_64-v3 as an input.

The attached diff has 2 parts, the first wraps the go binary (and only
the go binary) with GOAMD or the like. The second part is commented out,
but is how I would've fixed the extra 'set-microarchitecture phase.

I'm pretty certain that I have the logic correct, but I'm not certain
that it's being applied. It probably needs (system* "export" "GOAMD"
#$psabi) or something similar, when I tried adjusting syncthing to
display (getenv "GOAMD") I was getting #f.

-- 
Efraim Flashner   <efraim@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

[-- Attachment #1.2: wrap-go-binary.diff --]
[-- Type: text/plain, Size: 5183 bytes --]

diff --git a/guix/transformations.scm b/guix/transformations.scm
index 92d9c89c0e..0665f33178 100644
--- a/guix/transformations.scm
+++ b/guix/transformations.scm
@@ -441,6 +441,9 @@ (define tuning-compiler
       #~(begin
           (use-modules (ice-9 match))
 
+          (define psabi #$(gcc-architecture->micro-architecture-level
+                            micro-architecture))
+
           (define* (search-next command
                                 #:optional
                                 (path (string-split (getenv "PATH")
@@ -469,10 +472,25 @@ (define tuning-compiler
              (match (search-next (basename command))
                (#f (exit 127))
                (next
-                (apply execl next
+                 (if (and (search-next "go")
+                          (string=? next (search-next "go")))
+                   (cond
+                     ((string-prefix? "arm" psabi)
+                      (setenv "GOARM" (string-take-right psabi 1)))
+                     ((string-prefix? "powerpc" psabi)
+                      (setenv "GOPPC64" psabi))
+                     ((string-prefix? "x86_64" psabi)
+                      (setenv "GOAMD" (string-take-right psabi 2)))
+                     (else #t))
+                   '())
+                (apply
+                  execl next
                        (append (cons next arguments)
+                         (if (and (search-next "go")
+                                  (string=? next (search-next "go")))
+                           '()
                            (list (string-append "-march="
-                                                #$micro-architecture))))))))))
+                                                #$micro-architecture)))))))))))
 
     (define program
       (program-file (string-append "tuning-compiler-wrapper-" micro-architecture)
@@ -489,7 +507,8 @@ (define tuning-compiler
                          (for-each (lambda (program)
                                      (symlink #$program
                                               (string-append bin "/" program)))
-                                   '("cc" "gcc" "clang" "g++" "c++" "clang++")))))))
+                                   '("cc" "gcc" "clang" "g++" "c++" "clang++"
+                                     "go")))))))
 
 (define (build-system-with-tuning-compiler bs micro-architecture)
   "Return a variant of BS, a build system, that ensures that the compiler that
@@ -564,27 +583,31 @@ (define (build-system-with-tuning-compiler bs micro-architecture)
 
       (bag
         (inherit lowered)
-        (arguments
+        #;(arguments
           (substitute-keyword-arguments (bag-arguments lowered)
           ;; We add the tuning parameter after the default GO flags are set.
           ((#:phases phases '%standard-phases)
-             #~(modify-phases #$phases
-                 (add-after 'setup-go-environment 'set-microarchitecture
-                   (lambda _
-                     (cond
-                       ((string-prefix? "arm" #$psabi)
-                        (setenv "GOARM" (string-take-right #$psabi 1))
-                        (format #t "Setting GOARM to ~s."
-                                (getenv "GOARM")))
-                       ((string-prefix? "powerpc" #$psabi)
-                        (setenv "GOPPC64" #$psabi)
-                        (format #t "Setting GOPPC64 to ~s."
-                                (getenv "GOPPC64")))
-                       ((string-prefix? "x86_64" #$psabi)
-                        (setenv "GOAMD" (string-take-right #$psabi 2))
-                        (format #t "Setting GOAMD to ~s.\n"
-                                (getenv "GOAMD")))
-                       (else #t))))))))
+             ;; This phase is only in the go-build-system.
+             #~(if (assoc-ref #$phases 'setup-go-environment)
+                 (modify-phases #$phases
+                   (replace 'setup-go-environment
+                     (lambda* args
+                       (apply (assoc-ref #$phases 'setup-go-environment) args)
+                       (cond
+                         ((string-prefix? "arm" #$psabi)
+                          (setenv "GOARM" (string-take-right #$psabi 1))
+                          (format #t "Setting GOARM to ~s."
+                                  (getenv "GOARM")))
+                         ((string-prefix? "powerpc" #$psabi)
+                          (setenv "GOPPC64" #$psabi)
+                          (format #t "Setting GOPPC64 to ~s."
+                                  (getenv "GOPPC64")))
+                         ((string-prefix? "x86_64" #$psabi)
+                          (setenv "GOAMD" (string-take-right #$psabi 2))
+                          (format #t "Setting GOAMD to ~s.\n"
+                                  (getenv "GOAMD")))
+                         (else #t)))))
+                 #$phases))))
         (build-inputs
          ;; Arrange so that the compiler wrapper comes first in $PATH.
          `(("tuning-compiler" ,(tuning-compiler micro-architecture))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]