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

Default target-abi based on RISC-V extensions and user selectable -mabi/target-abi #10006

Merged
merged 3 commits into from
Dec 4, 2021

Conversation

akovaski
Copy link

@akovaski akovaski commented Oct 23, 2021

Fixes #9760
Zig now calculates a default -mabi/target-abi for the target RISC-V extensions. Zig will then pass the default target-abi to clang/llvm for Assembly, C (and Cpp, m, h), and Zig files.

This change also allows the user to manually select the target-abi for compilation. Following Clang and LLVM's example, the target-abi is separate from the triplet abi. Target-abi can be passed from the command line, like:

zig build-obj -target riscv64-freestanding -mabi lp64f main.zig

Or provided in build.zig when running zig build via the target_abi member of LibExeObjStep, like:

exe.target_abi = .lp64f;

@akovaski
Copy link
Author

Looking at LLVM internals, LLVM has separate psAbi values for a few different targets.

llvm/lib/Target/ARM/ARMTargetMachine.cpp: computeTargetABI -> ARMBaseTargetMachine::ARMABI
llvm/lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp: computeTargetABI -> MipsABIInfo
llvm/lib/Target/PowerPC/PPCTargetMachine.cpp: computeTargetABI -> PPCTargetMachine::PPCABI
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp: RISCVABI::ABI computeTargetABI

TargetAbi might be a more consistent name to chose rather than PsAbi which I currently use. Clang and LLVM seem to keep the target-triplet abi and the target-api separate, so I think that'll be fine for Zig as well.

Clang has a slightly larger number of classes that have the getABI function:

clang/lib/Basic/Targets/AArch64.cpp
clang/lib/Basic/Targets/ARM.cpp
clang/lib/Basic/Targets/Mips.h
clang/lib/Basic/Targets/PPC.h
clang/lib/Basic/Targets/RISCV.h
clang/lib/Basic/Targets/SystemZ.h
clang/lib/Basic/Targets/WebAssembly.cpp
clang/lib/Basic/Targets/X86.h

Conceptually it might make sense to separate the TargetAbi for each architecture. It could look something like:

pub const TargetAbi = union(Arch) {
    //TODO add ARM, Mips, and PowerPC
    riscv32: Riscv32ABI,
    riscv64: Riscv64ABI,
    // everything else: void,

    pub const Riscv32ABI = enum {
        ilp32e,
        ilp32,
        ilp32f,
        ilp32d,
        pub fn default(features: Cpu.Feature.Set) Riscv32ABI {
            if (riscv.featureSetHas(features, .d)) {
                return .ilp32d;
            } else if (riscv.featureSetHas(features, .f)) {
                return .ilp32f;
            } else {
                return .ilp32;
            }
        }
    };
    pub const Riscv64ABI = enum {
        lp64,
        lp64f,
        lp64d,
        pub fn default(features: Cpu.Feature.Set) ?Riscv64ABI {
            if (riscv.featureSetHas(features, .d)) {
                return .lp64d;
            } else if (riscv.featureSetHas(features, .f)) {
                return .lp64f;
            } else {
                return .lp64;
            }
        }
    };
    pub fn default(cpu: Cpu) ?TargetAbi {
        switch (cpu.arch) {
            .riscv32 => return .{ .riscv32 = Riscv32ABI.default(cpu.features) },
            .riscv64 => return .{ .riscv64 = Riscv64ABI.default(cpu.features) },
            else => return null,
        }
    }
    pub fn string(self: TargetAbi) ?[*:0]const u8 {
        switch (self) {
            .riscv32 => |abi| return @tagName(abi),
            .riscv64 => |abi| return @tagName(abi),
            else => return null,
        }
    }
};

@akovaski
Copy link
Author

Taking a look at the defaults noted in clang/lib/Driver/ToolChains/Arch/RISCV.cpp: getRISCVABI

  // GCC's logic around choosing a default `-mabi=` is complex. If GCC is not
  // configured using `--with-abi=`, then the logic for the default choice is
  // defined in config.gcc. This function is based on the logic in GCC 9.2.0.
  //
  // The logic used in GCC 9.2.0 is the following, in order:
  // 1. Explicit choices using `--with-abi=`
  // 2. A default based on `--with-arch=`, if provided
  // 3. A default based on the target triple's arch
  //
  // The logic in config.gcc is a little circular but it is not inconsistent.
  //
  // Clang does not have `--with-arch=` or `--with-abi=`, so we use `-march=`
  // and `-mabi=` respectively instead.
  //
  // In order to make chosing logic more clear, Clang uses the following logic,
  // in order:
  // 1. Explicit choices using `-mabi=`
  // 2. A default based on the architecture as determined by getRISCVArch
  // 3. Choose a default based on the triple

  // 1. If `-mabi=` is specified, use it.

  // 2. Choose a default based on the target architecture.
  //
  // rv32g | rv32*d -> ilp32d
  // rv32e -> ilp32e
  // rv32* -> ilp32
  // rv64g | rv64*d -> lp64d
  // rv64* -> lp64

  // 3. Choose a default based on the triple
  //
  // We deviate from GCC's defaults here:
  // - On `riscv{XLEN}-unknown-elf` we use the integer calling convention only.
  // - On all other OSs we use the double floating point calling convention.

I think Clang needs step 3 because it then uses the mabi value to decide march if march is not set.
Zig already has a default Cpu for each Cpu.Arch. I feel like it isn't necessary to add RISCV ISA extensions to the default Cpu; in the first place, Zig's default Cpu already has the D extension. Just fail the compile if mabi and the Cpu aren't compatible!

@akovaski
Copy link
Author

Made Target.default return a non-optional. It would be nice if Zig's switch statement was smart enough for TargetAbi.default to be defined as:

pub fn default(cpu: Cpu) TargetAbi {
    return switch (cpu.arch) {
        .riscv32 => TargetAbi{ .riscv32 = Riscv32ABI.default(cpu.features) },
        .riscv64 => TargetAbi{ .riscv64 = Riscv64ABI.default(cpu.features) },
        else => cpu.arch,
    };
}

Also, I updated TargetAbi.default to better match Clang's getRISCVABI step 2.

// Clang's getRISCVABI Step 2
bool HasD = ISAInfo->hasExtension("d");
unsigned XLen = ISAInfo->getXLen();
if (XLen == 32) {
  bool HasE = ISAInfo->hasExtension("e");
  if (HasD)
    return "ilp32d";
  if (HasE)
    return "ilp32e";
  return "ilp32";
} else if (XLen == 64) {
  if (HasD)
    return "lp64d";
  return "lp64";
}
llvm_unreachable("unhandled XLen");

@akovaski
Copy link
Author

akovaski commented Oct 25, 2021

Fixed issue with passing the target-abi to LLVM for Zig file compilation. Added ability for user to pass mabi.

@akovaski akovaski changed the title Default -mabi based on RISC-V extensions Default target-abi based on RISC-V extensions and user selectable -mabi/target-abi Oct 25, 2021
@akovaski
Copy link
Author

  • Moved target_abi from CrossTarget to LibExeObjStep for zig build.
  • Simplified the TargetAbi type a little bit and added some validation.
  • rebased and squashed

@akovaski akovaski marked this pull request as ready for review October 30, 2021 15:01
lib/std/target.zig Outdated Show resolved Hide resolved
lib/std/target.zig Outdated Show resolved Hide resolved
src/Compilation.zig Outdated Show resolved Hide resolved
@akovaski
Copy link
Author

There is a similar issue when compiling with release modes that are not Debug. I describe it back in a comment in the original issue #9760 (comment)

This is a problem with clang/LLVM and RISC-V LTO and mabi/target-abi.
I could 'fix' the issue in Zig by defaulting LTO to off for riscv architectures, but that might just be unnecessary noise in the compiler source, I dunno. The manual workaround of setting want_lto to false when building my riscv64 application works just as well.

diff --git a/src/Compilation.zig b/src/Compilation.zig
index e17493719..1089894c5 100644
--- a/src/Compilation.zig
+++ b/src/Compilation.zig
@@ -969,22 +969,24 @@ pub fn create(gpa: *Allocator, options: InitOptions) !*Compilation {
         const lto = blk: {
             if (options.want_lto) |explicit| {
                 if (!use_lld and !options.target.isDarwin())
                     return error.LtoUnavailableWithoutLld;
                 break :blk explicit;
             } else if (!use_lld) {
                 // TODO zig ld LTO support
                 // See https://github.com/ziglang/zig/issues/8680
                 break :blk false;
             } else if (options.c_source_files.len == 0) {
                 break :blk false;
+            } else if (options.target.cpu.arch == .riscv32 or options.target.cpu.arch == .riscv64) {
+                break :blk false;
             } else switch (options.output_mode) {
                 .Lib, .Obj => break :blk false,
                 .Exe => switch (options.optimize_mode) {
                     .Debug => break :blk false,
                     .ReleaseSafe, .ReleaseFast, .ReleaseSmall => break :blk true,
                 },
             }
         };

vole-dev and others added 3 commits December 3, 2021 16:53
The target abi can also be set in build.zig via LibExeObjStep.target_abi

The value passed in is checked that it is a valid value in
std.Target.TargetAbi

The target abi is also validated against the target cpu
This branch introduced std.Target.TargetAbi when we already had
std.Target.Abi which was, unsurprisingly, already suited for this task.

Also pull out the -mabi= cc flag addition to the common area instead of
duplicating it for assembly and c files.
@andrewrk andrewrk merged commit 3311ef3 into ziglang:master Dec 4, 2021
@andrewrk
Copy link
Member

andrewrk commented Dec 4, 2021

I made some changes to this branch in 4c1a623 before merging.

The changes that landed are much less drastic. If we need to add additional std.Target.Abi enum tags to handle additional possibilities, let's discuss that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

riscv64-freestanding: C and Zig object files default to soft-float ABI
3 participants