diff mbox series

[bug#69581,07/11] gnu: rust: Add tuning information.

Message ID b6f27a7aba739de0e1bc7437a050e5104065482f.1709722620.git.efraim@flashner.co.il
State New
Headers show
Series CPU tuning patches | expand

Commit Message

Efraim Flashner March 6, 2024, 11:06 a.m. UTC 
* gnu/packages/rust.scm (rust)[properties]: Add clang-properties
matching the input llvm package.

Change-Id: Ie2ef2387fff8aa639dcd73752bcaf3c26bbb376d
---
 gnu/packages/rust.scm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ludovic Courtès March 6, 2024, 6:09 p.m. UTC | #1 
Efraim Flashner <efraim@flashner.co.il> skribis:

> * gnu/packages/rust.scm (rust)[properties]: Add clang-properties
> matching the input llvm package.
>
> Change-Id: Ie2ef2387fff8aa639dcd73752bcaf3c26bbb376d

Hmm rustc actually uses LLVM for code generation?  I’m a bit lost.
Anyway, if it supports those same flags as Clang, all good; maybe add a
comment to say so explicitly.

> +      (properties `(`(append
> +                       ,(alist-delete 'hidden? (package-properties base-rust))
> +                       ,@(clang-properties "15"))))

I think you meant:

  (properties (append (alist-delete 'hidden? (package-properties base-rust))
                      (clang-properties "15")))

Ludo’.
Ludovic Courtès March 6, 2024, 6:10 p.m. UTC | #2 
Efraim Flashner <efraim@flashner.co.il> skribis:

> +      (properties `(`(append
> +                       ,(alist-delete 'hidden? (package-properties base-rust))
> +                       ,@(clang-properties "15"))))

Just realized that we cannot really call ‘clang-properties’ from here
because ‘properties’ is not a thunked field, so this would lead to a
top-level circular dependency…  You might need to duplicate the info.

Thanks,
Ludo’.
Efraim Flashner March 7, 2024, 9:09 a.m. UTC | #3 
On Wed, Mar 06, 2024 at 07:09:32PM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > * gnu/packages/rust.scm (rust)[properties]: Add clang-properties
> > matching the input llvm package.
> >
> > Change-Id: Ie2ef2387fff8aa639dcd73752bcaf3c26bbb376d
> 
> Hmm rustc actually uses LLVM for code generation?  I’m a bit lost.
> Anyway, if it supports those same flags as Clang, all good; maybe add a
> comment to say so explicitly.

It supports the same target_cpu targets as clang does. With llvm
switching to a monorepo they've apparently done a better job about
syncing the list between llvm and clang.

> > +      (properties `(`(append
> > +                       ,(alist-delete 'hidden? (package-properties base-rust))
> > +                       ,@(clang-properties "15"))))
> 
> I think you meant:
> 
>   (properties (append (alist-delete 'hidden? (package-properties base-rust))
>                       (clang-properties "15")))

I would love to have it cleaner like this.  There was a lot of trial and
error to end up with what I had above.  I'll try that.
Efraim Flashner March 7, 2024, 9:10 a.m. UTC | #4 
On Wed, Mar 06, 2024 at 07:10:54PM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > +      (properties `(`(append
> > +                       ,(alist-delete 'hidden? (package-properties base-rust))
> > +                       ,@(clang-properties "15"))))
> 
> Just realized that we cannot really call ‘clang-properties’ from here
> because ‘properties’ is not a thunked field, so this would lead to a
> top-level circular dependency…  You might need to duplicate the info.

We also use the list for zig.

Considering we're only supporting one version for rust duplicating the
information wouldn't be the worst thing but it'd be better to only have
it declared in one spot.
Ludovic Courtès March 7, 2024, 9:38 p.m. UTC | #5 
Efraim Flashner <efraim@flashner.co.il> skribis:

> On Wed, Mar 06, 2024 at 07:10:54PM +0100, Ludovic Courtès wrote:
>> Efraim Flashner <efraim@flashner.co.il> skribis:
>> 
>> > +      (properties `(`(append
>> > +                       ,(alist-delete 'hidden? (package-properties base-rust))
>> > +                       ,@(clang-properties "15"))))
>> 
>> Just realized that we cannot really call ‘clang-properties’ from here
>> because ‘properties’ is not a thunked field, so this would lead to a
>> top-level circular dependency…  You might need to duplicate the info.
>
> We also use the list for zig.
>
> Considering we're only supporting one version for rust duplicating the
> information wouldn't be the worst thing but it'd be better to only have
> it declared in one spot.

Then it should be declared in a module that’s not in a cycle with its
users (that’s annoying!).  Maybe a separate ‘llvm-meta.scm’?

Ludo’.
Efraim Flashner March 8, 2024, 12:05 p.m. UTC | #6 
On Thu, Mar 07, 2024 at 10:38:06PM +0100, Ludovic Courtès wrote:
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > On Wed, Mar 06, 2024 at 07:10:54PM +0100, Ludovic Courtès wrote:
> >> Efraim Flashner <efraim@flashner.co.il> skribis:
> >> 
> >> > +      (properties `(`(append
> >> > +                       ,(alist-delete 'hidden? (package-properties base-rust))
> >> > +                       ,@(clang-properties "15"))))
> >> 
> >> Just realized that we cannot really call ‘clang-properties’ from here
> >> because ‘properties’ is not a thunked field, so this would lead to a
> >> top-level circular dependency…  You might need to duplicate the info.
> >
> > We also use the list for zig.
> >
> > Considering we're only supporting one version for rust duplicating the
> > information wouldn't be the worst thing but it'd be better to only have
> > it declared in one spot.
> 
> Then it should be declared in a module that’s not in a cycle with its
> users (that’s annoying!).  Maybe a separate ‘llvm-meta.scm’?

How about moving it into (guix cpu) together with the ones from gcc?
There are apparently some CPUs that are identical but have different
names that we could map together and would allow us to make any changes
to them in one place.  We've already pretty much done that with the
search paths.
Ludovic Courtès March 8, 2024, 9:57 p.m. UTC | #7 
Hi,

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

> On Thu, Mar 07, 2024 at 10:38:06PM +0100, Ludovic Courtès wrote:

[...]

>> Then it should be declared in a module that’s not in a cycle with its
>> users (that’s annoying!).  Maybe a separate ‘llvm-meta.scm’?
>
> How about moving it into (guix cpu) together with the ones from gcc?
> There are apparently some CPUs that are identical but have different
> names that we could map together and would allow us to make any changes
> to them in one place.  We've already pretty much done that with the
> search paths.

To me, the whole point of the ‘compiler-cpu-architectures’ property is
that this info can be stored in the compiler package itself rather than
in some remote unrelated place (the same goes for search paths).  My
instinct would be to preserve that, hence the suggestion of a new (gnu
packages llvm-meta) or (… llvm-infra) module, something like that.

We should also keep in mind that those CPU names are those defined by
compilers themselves; they don’t have to match the vendor-chosen name or
the name chosen by some other compiler.

WDYT?  Does that make sense?

Ludo’.
Efraim Flashner March 10, 2024, 8:26 a.m. UTC | #8 
On Fri, Mar 08, 2024 at 10:57:03PM +0100, Ludovic Courtès wrote:
> Hi,
> 
> Efraim Flashner <efraim@flashner.co.il> skribis:
> 
> > On Thu, Mar 07, 2024 at 10:38:06PM +0100, Ludovic Courtès wrote:
> 
> [...]
> 
> >> Then it should be declared in a module that’s not in a cycle with its
> >> users (that’s annoying!).  Maybe a separate ‘llvm-meta.scm’?
> >
> > How about moving it into (guix cpu) together with the ones from gcc?
> > There are apparently some CPUs that are identical but have different
> > names that we could map together and would allow us to make any changes
> > to them in one place.  We've already pretty much done that with the
> > search paths.
> 
> To me, the whole point of the ‘compiler-cpu-architectures’ property is
> that this info can be stored in the compiler package itself rather than
> in some remote unrelated place (the same goes for search paths).  My
> instinct would be to preserve that, hence the suggestion of a new (gnu
> packages llvm-meta) or (… llvm-infra) module, something like that.
> 
> We should also keep in mind that those CPU names are those defined by
> compilers themselves; they don’t have to match the vendor-chosen name or
> the name chosen by some other compiler.
> 
> WDYT?  Does that make sense?

That makes sense to me. I'm also going to look at moving
system->llvm-target to llvm-meta.  I think its used with dlang.scm and
maybe elsewhere, or could be used at least.  All these languages using
llvm as a backend means we keep on wanting to use these functions
elsewhere.
diff mbox series

Patch

diff --git a/gnu/packages/rust.scm b/gnu/packages/rust.scm
index 1f129a93bd..68917ee5e8 100644
--- a/gnu/packages/rust.scm
+++ b/gnu/packages/rust.scm
@@ -964,7 +964,9 @@  (define-public rust
   (let ((base-rust rust-1.75))
     (package
       (inherit base-rust)
-      (properties (alist-delete 'hidden? (package-properties base-rust)))
+      (properties `(`(append
+                       ,(alist-delete 'hidden? (package-properties base-rust))
+                       ,@(clang-properties "15"))))
       (outputs (cons* "rust-src" "tools" (package-outputs base-rust)))
       (source
        (origin