diff mbox

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

Message ID ZLZ00y3U8vekwGNF@3900XT
State New
Headers show

Commit Message

Efraim Flashner July 18, 2023, 11:17 a.m. UTC 
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.

Comments

Josselin Poiret July 19, 2023, 8:39 a.m. UTC | #1 
Hi Efraim,

Efraim Flashner <efraim@flashner.co.il> writes:

> 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.

That system* call won't do anything: export is a built-in in most shells
that sets an environment variable for the current shell session.
system* doesn't run the command you're giving it through a shell, so it
won't find `export`, and if you use system instead (which would run it
through a shell) it will only take effect during the (system ...) call,
after which the shell session is torn down.  You really need to use the
guile setenv if you want it to have any effect on started programs (or
use the spawn interface to specify env variables for a specific
invocation).

Best,
Ludovic Courtès Aug. 7, 2023, 7:33 a.m. UTC | #2 
Hi,

Efraim Flashner <efraim@flashner.co.il> skribis:

>> 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.

At first sight it seems to me like it’s going in the right direction.

Perhaps we should create a separate ‘tuning-go-compiler’ for clarity,
and arrange to factorize ‘search-next’ between the two.

Let me know what you think!

Thanks,
Ludo’.
Ludovic Courtès Aug. 21, 2023, 4:54 p.m. UTC | #3 
Hi Efraim,

Ludovic Courtès <ludovic.courtes@inria.fr> skribis:

> Efraim Flashner <efraim@flashner.co.il> skribis:
>
>>> 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.
>
> At first sight it seems to me like it’s going in the right direction.
>
> Perhaps we should create a separate ‘tuning-go-compiler’ for clarity,
> and arrange to factorize ‘search-next’ between the two.
>
> Let me know what you think!

I see you pushed a variant as 1fd4f544b3065af225731462f3d3d647da781ee8,
neat.

Closing!

Ludo’.
diff mbox

Patch

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))