Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove the concept of "sub-architecture" in favor of CPU features #4261

Closed
andrewrk opened this issue Jan 21, 2020 · 8 comments · Fixed by #4509
Closed

remove the concept of "sub-architecture" in favor of CPU features #4261

andrewrk opened this issue Jan 21, 2020 · 8 comments · Fixed by #4509
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@andrewrk
Copy link
Member

the "sub-architecture" part of the target overlaps with specifying target CPU features:

    pub const Arch = union(enum) {
        arm: Arm32,
        armeb: Arm32,
        aarch64: Arm64,
        aarch64_be: Arm64,
        aarch64_32: Arm64,
// ....
        pub const Arm64 = enum {
            v8_5a,
            v8_4a,
            v8_3a,
            v8_2a,
            v8_1a,
            v8,
            v8r,
            v8m_baseline,
            v8m_mainline,
        };

arm target features:

const std = @import("../std.zig");
const Cpu = std.Target.Cpu;

pub const Feature = enum {
    @"32bit",
    @"8msecext",
    a12,
    a15,
    a17,
    a32,
    a35,
    a5,
    a53,
    a55,
    a57,
    a7,
    a72,
    a73,
    a75,
    a76,
    a8,
    a9,
    aclass,
    acquire_release,
    aes,
    armv2,
    armv2a,
    armv3,
    armv3m,
    armv4,
    armv4t,
    armv5t,
    armv5te,
    armv5tej,
    armv6,
    armv6_m,
    armv6j,
    armv6k,
    armv6kz,
    armv6s_m,
    armv6t2,
    armv7_a,
    armv7_m,
    armv7_r,
    armv7e_m,
    armv7k,
    armv7s,
    armv7ve,
    armv8_a,
    armv8_m_base,
    armv8_m_main,
    armv8_r,
    armv8_1_a,
    armv8_1_m_main,
    armv8_2_a,
    armv8_3_a,
    armv8_4_a,
    armv8_5_a,

I propose to entirely remove "sub-architecture" concept from the language & CLI, and rely instead on the more-powerful ability to specify CPU features. Each "sub-architecture" is represented as a CPU feature that depends on the previous one.

@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Jan 21, 2020
@andrewrk andrewrk added this to the 0.6.0 milestone Jan 21, 2020
@frmdstryr
Copy link
Contributor

This sounds like it's mixing cpu's, arches, and features.

The way I understand it is a given arm core/cpu, eg the cortex-m7, only has one arch (ex armv7em) but the arch can be backwards compatible with previous arches. Then the features (eg FPU, MPU) depend on options the core and/or arch supports and the actual part number / hw config.

See https://en.wikipedia.org/wiki/List_of_ARM_microarchitectures and https://en.wikipedia.org/wiki/ARM_Cortex-M.

All the cortex-a7, cortex-a9, etc.. are specific arm cores that should each be their own Cpu that maps to a specific arch and set of optional but supported features.

At the end of the day it should be possible to target a specific arch + features or a specific cpu + features as you can already do with gcc https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html

@andrewrk
Copy link
Member Author

@frmdstryr everything in your comment here is compatible with this proposal

@frmdstryr
Copy link
Contributor

Cool, ship it.

#4264 is working fine for me (that PR allowed me to enable the FPU) so if anything breaks I'll let you know.

@andrewrk
Copy link
Member Author

andrewrk commented Feb 19, 2020

Here's what it will come down to for Zig programmers:

use case 1, specifying a std.Target programmatically:

             .target = Target{
                 .Cross = CrossTarget{
+                    .cpu = Target.Cpu.baseline(.x86_64),
                     .os = .linux,
-                    .arch = .x86_64,
                     .abi = .none,
-                    .cpu_features = Target.Arch.x86_64.getBaselineCpuFeatures(),
                 },
             },
             .target = Target{
                 .Cross = CrossTarget{
+                    .cpu = Target.Cpu.baseline(.aarch64),
                     .os = .linux,
-                    .arch = Target.Arch{ .aarch64 = .v8a },
-                    .cpu_features = (Target.Arch{ .aarch64 = .v8a }).getBaselineCpuFeatures(),
                     .abi = .musl,
                 },
             },
-            .target = Target{
-                .Cross = CrossTarget{
-                    .os = .linux,
-                    .arch = Target.Arch{ .arm = .v8a },
-                    .cpu_features = (Target.Arch{ .arm = .v8a }).getBaselineCpuFeatures(),
-                    .abi = .none,
-                },
-            },
+            .target = Target.parse(.{
+                .arch_os_abi = "arm-linux-none",
+                .cpu_features = "generic+v8a",
+            }) catch unreachable,
         },

use case 2, specifying targets on the comand line:

# -target-cpu renamed to -mcpu to conform to established conventions,
# and it gains additional power to support cpu features
-target x86_64-linux-gnu -mcpu=native+avx512f-fast_scalar_fsqrt
-target x86_64-linux-gnu -mcpu=x86_64-sse2
-target i386-linux-gnu -mcpu=pentium+sse2-slow_unaligned_mem_16

# with no -mcpu argument, chooses the "baseline" CPU which is generic+v6m
-target arm-linux-gnu

-target armv4t-freestanding # error, no longer supported
-target arm-freestanding -mcpu=generic+v4t # do this instead

andrewrk added a commit that referenced this issue Feb 20, 2020
in favor of CPU features. Also rearrange the `std.Target`
data structure.

 * note: `@import("builtin")` was already deprecated in favor of
   `@import("std").builtin`.
 * `std.builtin.arch` is now deprecated in favor of
   `std.builtin.cpu.arch`.
 * `std.Target.CpuFeatures.Cpu` is now `std.Target.Cpu.Model`.
 * `std.Target.CpuFeatures` is now `std.Target.Cpu`.
 * `std.Target` no longer has an `arch` field. Instead it has a
   `cpu` field, which has `arch`, `model`, and `features`.
 * `std.Target` no longer has a `cpu_features` field.
 * `std.Target.Arch` is moved to `std.Target.Cpu.Arch` and
   it is an enum instead of a tagged union.
 * `std.Target.parseOs` is moved to `std.Target.Os.parse`.
 * `std.Target.parseAbi` is moved to `std.Target.Abi.parse`.
 * `std.Target.parseArchSub` is only for arch now and moved
    to `std.Target.Cpu.Arch.parse`.
 * `std.Target.parse` is improved to accept CPU name and features.
 * `std.Target.Arch.getBaselineCpuFeatures` is moved to
   `std.Target.Cpu.baseline`.
 * `std.Target.allCpus` is renamed to `std.Target.allCpuModels`.
 * `std.Target.defaultAbi` is moved to `std.Target.Abi.default`.
 * Significant cleanup of aarch64 and arm CPU features, resulting in
   the needed bit count for cpu feature set going from 174 to 138.
 * Add `std.Target.Cpu.Feature.Set.addFeatureSet` for merging
   feature sets together.

`-target-feature` and `-target-cpu` are removed in favor of
`-mcpu`, to conform to established conventions, and it gains
additional power to support cpu features. The syntax is:
-mcpu=name+on1+on2-off1-off2

closes #4261
@frmdstryr
Copy link
Contributor

Looks like a nice cleanup.

@frmdstryr
Copy link
Contributor

So this

    const arch = builtin.Arch{ .thumb = .v7em };
    const cpu = &builtin.Target.arm.cpu.cortex_m7;
    const elf = b.addExecutable("firmware.elf", "startup.zig");
    elf.setTheTarget(builtin.Target{
        .Cross = .{
            .arch = arch,
            .os = .freestanding,
            .abi = .none,
            .cpu_features = builtin.Target.CpuFeatures.initFromCpu(arch, cpu),
        },
    });

is now:

    const elf = b.addExecutable("firmware.elf", "startup.zig");
    elf.setTheTarget(builtin.Target.parse(.{
        .arch_os_abi="thumb-freestanding-none",
        .cpu_features="cortex_m7",
    }) catch unreachable);

and it builds and runs fine (with the fpu), thanks!

@frmdstryr
Copy link
Contributor

For consistency it would be nice if it worked with either underscores or dashes, as cortex_m7 works but cortex-m7 (the llvm_name) errors with .UnknownCpu

@daurnimator
Copy link
Contributor

    elf.setTheTarget(builtin.Target.parse(.{

builtin is deprecated; use std.builtin instead.... but also std.builtin.Target is deprecated, use std.Target instead :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted This proposal is planned. breaking Implementing this issue could cause existing code to no longer compile or have different behavior. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants