| Message ID | f0435844501301f1850deffe64d3ac254007f4c3.1687247150.git.efraim@flashner.co.il |
|---|---|
| State | New |
| Headers | show |
| Series | More package tuning | expand |
Efraim Flashner <efraim@flashner.co.il> skribis: > * guix/transformations.scm (tuned-package)[build-system]: Don't replace > the build-system if inheriting from the go-build-system. > [arguments]: If using the go-build-system add a phase to set the > micro-architecture for Go. [...] > @@ -570,14 +571,42 @@ (define (tuned-package p micro-architecture) > (package > (inherit p) > (build-system > - (build-system-with-tuning-compiler (package-build-system p) > - micro-architecture)) > + ;; The go compiler doesn't need to be wrapped. > + ;; XXX: This is where we check for supported micro-architectures. > + (if (eq? (build-system-name (package-build-system p)) > + 'go) > + (package-build-system p) > + (build-system-with-tuning-compiler (package-build-system p) > + micro-architecture))) > (arguments > ;; The machine building this package may or may not be able to run code > ;; for MICRO-ARCHITECTURE. Because of that, skip tests; they are run for > ;; the "baseline" variant anyway. > (substitute-keyword-arguments (package-arguments p) > - ((#:tests? _ #f) #f))) > + ((#:tests? _ #f) #f) > + ;; We add the tuning parameter after the default GO flags are set. > + ((#:phases phases '%standard-phases) > + (if (eq? (build-system-name (package-build-system p)) > + 'go) > + #~(modify-phases #$phases > + (add-after 'setup-go-environment 'set-microarchitecture Can we use the same approach as before with ‘build-system-with-tuning-compiler’? That seems more robust to me and it’d be more consistent. Ludo’.
On Sun, Jun 25, 2023 at 10:52:14PM +0200, Ludovic Courtès wrote: > Efraim Flashner <efraim@flashner.co.il> skribis: > > > * guix/transformations.scm (tuned-package)[build-system]: Don't replace > > the build-system if inheriting from the go-build-system. > > [arguments]: If using the go-build-system add a phase to set the > > micro-architecture for Go. > > [...] > > > @@ -570,14 +571,42 @@ (define (tuned-package p micro-architecture) > > (package > > (inherit p) > > (build-system > > - (build-system-with-tuning-compiler (package-build-system p) > > - micro-architecture)) > > + ;; The go compiler doesn't need to be wrapped. > > + ;; XXX: This is where we check for supported micro-architectures. > > + (if (eq? (build-system-name (package-build-system p)) > > + 'go) > > + (package-build-system p) > > + (build-system-with-tuning-compiler (package-build-system p) > > + micro-architecture))) > > (arguments > > ;; The machine building this package may or may not be able to run code > > ;; for MICRO-ARCHITECTURE. Because of that, skip tests; they are run for > > ;; the "baseline" variant anyway. > > (substitute-keyword-arguments (package-arguments p) > > - ((#:tests? _ #f) #f))) > > + ((#:tests? _ #f) #f) > > + ;; We add the tuning parameter after the default GO flags are set. > > + ((#:phases phases '%standard-phases) > > + (if (eq? (build-system-name (package-build-system p)) > > + 'go) > > + #~(modify-phases #$phases > > + (add-after 'setup-go-environment 'set-microarchitecture > > Can we use the same approach as before with > ‘build-system-with-tuning-compiler’? That seems more robust to me and > it’d be more consistent. I'll look to see how to move this into build-system-with-tuning-compiler. I think I previously got hung-up on the tuning-compiler function which wouldn't work for go. At first glance it looks like I could just lift-and-shift the extra phase over. I'll just have to make sure that either it's only for the go-build-system or occurs sometime after the setup-go-environment phase but before/after a phase which exists in all build systems. Then it's the toss-up between leaving it as-is and only occurring with the go-build-system or also occurring on any tuned package which also has go code.
Hi Efraim, Thanks for pushing this patch series past the finish line! Efraim Flashner <efraim@flashner.co.il> skribis: > I'll look to see how to move this into > build-system-with-tuning-compiler. I think I previously got hung-up on > the tuning-compiler function which wouldn't work for go. At first glance > it looks like I could just lift-and-shift the extra phase over. I'll > just have to make sure that either it's only for the go-build-system or > occurs sometime after the setup-go-environment phase but before/after a > phase which exists in all build systems. Then it's the toss-up between > leaving it as-is and only occurring with the go-build-system or also > occurring on any tuned package which also has go code. 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’.
On Thu, Jul 13, 2023 at 05:27:21PM +0200, Ludovic Courtès wrote: > Hi Efraim, > > Thanks for pushing this patch series past the finish line! > > Efraim Flashner <efraim@flashner.co.il> skribis: > > > I'll look to see how to move this into > > build-system-with-tuning-compiler. I think I previously got hung-up on > > the tuning-compiler function which wouldn't work for go. At first glance > > it looks like I could just lift-and-shift the extra phase over. I'll > > just have to make sure that either it's only for the go-build-system or > > occurs sometime after the setup-go-environment phase but before/after a > > phase which exists in all build systems. Then it's the toss-up between > > leaving it as-is and only occurring with the go-build-system or also > > occurring on any tuned package which also has go code. > > 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. 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 :)
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’?). Thanks in advance! Ludo’.
diff --git a/guix/transformations.scm b/guix/transformations.scm index a289f81219..47eb4b0515 100644 --- a/guix/transformations.scm +++ b/guix/transformations.scm @@ -2,6 +2,7 @@ ;;; Copyright © 2016-2023 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2021 Marius Bakke <marius@gnu.org> ;;; Copyright © 2023 Sarthak Shah <shahsarthakw@gmail.com> +;;; Copyright © 2023 Efraim Flashner <efraim@flashner.co.il> ;;; ;;; This file is part of GNU Guix. ;;; @@ -570,14 +571,42 @@ (define (tuned-package p micro-architecture) (package (inherit p) (build-system - (build-system-with-tuning-compiler (package-build-system p) - micro-architecture)) + ;; The go compiler doesn't need to be wrapped. + ;; XXX: This is where we check for supported micro-architectures. + (if (eq? (build-system-name (package-build-system p)) + 'go) + (package-build-system p) + (build-system-with-tuning-compiler (package-build-system p) + micro-architecture))) (arguments ;; The machine building this package may or may not be able to run code ;; for MICRO-ARCHITECTURE. Because of that, skip tests; they are run for ;; the "baseline" variant anyway. (substitute-keyword-arguments (package-arguments p) - ((#:tests? _ #f) #f))) + ((#:tests? _ #f) #f) + ;; We add the tuning parameter after the default GO flags are set. + ((#:phases phases '%standard-phases) + (if (eq? (build-system-name (package-build-system p)) + 'go) + #~(modify-phases #$phases + (add-after 'setup-go-environment 'set-microarchitecture + (lambda _ + (let ((microarch #$micro-architecture)) + (cond + ((string-prefix? "arm" microarch) + (setenv "GOARM" (string-take-right microarch 1)) + (format #t "Setting GOARM to ~s." + (getenv "GOARM"))) + ((string-prefix? "powerpc" microarch) + (setenv "GOPPC64" microarch) + (format #t "Setting GOPPC64 to ~s." + (getenv "GOPPC64"))) + ((string-prefix? "x86_64" microarch) + (setenv "GOAMD" (string-take-right microarch 2)) + (format #t "Setting GOAMD to ~s.\n" + (getenv "GOAMD"))) + (else #t)))))) + phases)))) (properties `((cpu-tuning . ,micro-architecture)