From patchwork Tue Jul 18 11:17:39 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Efraim Flashner X-Patchwork-Id: 51766 Return-Path: X-Original-To: patchwork@mira.cbaines.net Delivered-To: patchwork@mira.cbaines.net Received: by mira.cbaines.net (Postfix, from userid 113) id 98B9D27BBE9; Tue, 18 Jul 2023 12:18:18 +0100 (BST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on mira.cbaines.net X-Spam-Level: X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mira.cbaines.net (Postfix) with ESMTPS id CCCCE27BBE2 for ; Tue, 18 Jul 2023 12:18:17 +0100 (BST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qLiiT-0007ch-Ky; Tue, 18 Jul 2023 07:18:05 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qLiiQ-0007cJ-Np for guix-patches@gnu.org; Tue, 18 Jul 2023 07:18:03 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qLiiQ-0005LC-Fx for guix-patches@gnu.org; Tue, 18 Jul 2023 07:18:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qLiiQ-0006KR-Ag for guix-patches@gnu.org; Tue, 18 Jul 2023 07:18:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#64188] [PATCH 0/8] More package tuning Resent-From: Efraim Flashner Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Tue, 18 Jul 2023 11:18:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 64188 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Ludovic =?utf-8?q?Court=C3=A8s?= Cc: Josselin Poiret , Tobias Geerinckx-Rice , Simon Tournier , Mathieu Othacehe , Christopher Baines , 64188@debbugs.gnu.org, Ricardo Wurmus Received: via spool by 64188-submit@debbugs.gnu.org id=B64188.168967907124308 (code B ref 64188); Tue, 18 Jul 2023 11:18:02 +0000 Received: (at 64188) by debbugs.gnu.org; 18 Jul 2023 11:17:51 +0000 Received: from localhost ([127.0.0.1]:51704 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qLiiE-0006K0-O2 for submit@debbugs.gnu.org; Tue, 18 Jul 2023 07:17:51 -0400 Received: from mail-wm1-x32d.google.com ([2a00:1450:4864:20::32d]:52553) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qLiiC-0006Jk-60 for 64188@debbugs.gnu.org; Tue, 18 Jul 2023 07:17:49 -0400 Received: by mail-wm1-x32d.google.com with SMTP id 5b1f17b1804b1-3fba8e2aa52so57886585e9.1 for <64188@debbugs.gnu.org>; Tue, 18 Jul 2023 04:17:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689679062; x=1692271062; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:sender:from:to :cc:subject:date:message-id:reply-to; bh=+8e90LHLplUzpPF2kyU1dYM7L5uw2vEi6Sezqe98+m8=; b=aOdSU+ijn37+vB8Bl7iCg7S6pKMROmF8QE58YTlX4fUP6TiRNyACeA1Xrdt1wgO0eN doDOT8K/voVc7008jhjWh77rKqdPX0+DVcidRGyyScd2alVuFqHgipMiB9h78//e4n4K XkR6RJHraxEdJiDu4gRwJ6MQ9LnHSta8J2OjCW7ycHvctTDtADxY5e8YAcVe7TVcT5gN T3T5sm6LnfxqUA7kmq0dSJ3+/1TXoEG/KyWhgoYD8RBkUUJui90BeGexWnp+8DG4ERVC hmsq44EMivzTxGgGYQg8AMUpPb8/gSt2Rd25KeX5MeiR1bGc3fIMf2XizeKEM1sWN4cr QHaQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689679062; x=1692271062; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:sender :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=+8e90LHLplUzpPF2kyU1dYM7L5uw2vEi6Sezqe98+m8=; b=THXeuh6OUKgDv7LMjdt4FrLt5jQBuAjAGW8X3HG0elwnFoUEvjtm/+Uk5EZveAmiik bcQmMw8o6tTdgNj6S2yQJQdrIrkpOvlNICbUAsCn/8td9gudFtJ7EVo9o2CKVacGgaM/ ME9R8ir+hCv8dZSb1ME7WxVQzyu5p8FB4Jn9s4LPHJ3JuasLbESMF1PMaw5qr6ckE38C RxcQno38lN5nPN3CnAeiGTlB9xnJuJHVPktr8U/5YXlSqcJVWhJtCISQHcfU/Z832hWz MvJt+FRcGYHx4L87m/sSzod6pgmXIsNXfTFTtziIMFsi2LtJ2MJyF1wfh8UTxEB2vMiv Wkzg== X-Gm-Message-State: ABy/qLaNg/Jo/rPdXC5If1L3Udnghhk9+B6fmaI9pKmCtoD4LeEfePJV zhxUmm3o4WFJ78PrPZIu5xI= X-Google-Smtp-Source: APBJJlGKI5tTVRZdXoGgBmbKMavwhGjjmljIK4F5g/9fo6Cijc4JKJCxv7yski8/7o2HQgRjd5tCLw== X-Received: by 2002:a1c:e915:0:b0:3fb:e054:903f with SMTP id q21-20020a1ce915000000b003fbe054903fmr1677004wmc.36.1689679062110; Tue, 18 Jul 2023 04:17:42 -0700 (PDT) Received: from localhost ([2a02:ed3:914:a500:bdef:3269:70ba:bfc8]) by smtp.gmail.com with ESMTPSA id 25-20020a05600c025900b003fc00702f65sm9998426wmj.46.2023.07.18.04.17.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Jul 2023 04:17:41 -0700 (PDT) Date: Tue, 18 Jul 2023 14:17:39 +0300 From: Efraim Flashner Message-ID: Mail-Followup-To: Efraim Flashner , Ludovic =?utf-8?q?Court=C3=A8s?= , 64188@debbugs.gnu.org, Christopher Baines , Josselin Poiret , Mathieu Othacehe , Ricardo Wurmus , Simon Tournier , Tobias Geerinckx-Rice References: <87352fz2m9.fsf_-_@gnu.org> <871qhbzvae.fsf_-_@gnu.org> <87wmyypmts.fsf@inria.fr> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87wmyypmts.fsf@inria.fr> X-PGP-Key-ID: 0x41AAE7DCCA3D8351 X-PGP-Key: https://flashner.co.il/~efraim/efraim_flashner.asc X-PGP-Fingerprint: A28B F40C 3E55 1372 662D 14F7 41AA E7DC CA3D 8351 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org Sender: guix-patches-bounces+patchwork=mira.cbaines.net@gnu.org X-getmail-retrieved-from-mailbox: Patches On Mon, Jul 17, 2023 at 05:41:35PM +0200, Ludovic Courtès wrote: > Hi, > > Efraim Flashner 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. 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))