From f950d763a160d7ee333dde75ff61c9ac6597c803 Mon Sep 17 00:00:00 2001 From: Jacob G-W Date: Fri, 12 Nov 2021 18:10:13 -0500 Subject: [PATCH 1/4] stage2 x86_64 codegen: don't count return registers as callee-preserved --- src/arch/x86_64/CodeGen.zig | 4 ++-- src/arch/x86_64/bits.zig | 19 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index fac9a53a14..2de2796983 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -1902,10 +1902,10 @@ fn airCall(self: *Self, inst: Air.Inst.Index) !void { try self.register_manager.getReg(reg, null); try self.genSetReg(arg_ty, reg, arg_mcv); }, - .stack_offset => { + .stack_offset => |off| { // Here we need to emit instructions like this: // mov qword ptr [rsp + stack_offset], x - return self.fail("TODO implement calling with parameters in memory", .{}); + try self.genSetStack(arg_ty, off, arg_mcv); }, .ptr_stack_offset => { return self.fail("TODO implement calling with MCValue.ptr_stack_offset arg", .{}); diff --git a/src/arch/x86_64/bits.zig b/src/arch/x86_64/bits.zig index df221595f3..7b09dc59e7 100644 --- a/src/arch/x86_64/bits.zig +++ b/src/arch/x86_64/bits.zig @@ -84,15 +84,13 @@ pub const Register = enum(u7) { /// Returns the index into `callee_preserved_regs`. pub fn allocIndex(self: Register) ?u4 { return switch (self) { - .rax, .eax, .ax, .al => 0, - .rcx, .ecx, .cx, .cl => 1, - .rdx, .edx, .dx, .dl => 2, - .rsi, .esi, .si => 3, - .rdi, .edi, .di => 4, - .r8, .r8d, .r8w, .r8b => 5, - .r9, .r9d, .r9w, .r9b => 6, - .r10, .r10d, .r10w, .r10b => 7, - .r11, .r11d, .r11w, .r11b => 8, + .rcx, .ecx, .cx, .cl => 0, + .rsi, .esi, .si => 1, + .rdi, .edi, .di => 2, + .r8, .r8d, .r8w, .r8b => 3, + .r9, .r9d, .r9w, .r9b => 4, + .r10, .r10d, .r10w, .r10b => 5, + .r11, .r11d, .r11w, .r11b => 6, else => null, }; } @@ -145,7 +143,8 @@ pub const Register = enum(u7) { // zig fmt: on /// These registers belong to the called function. -pub const callee_preserved_regs = [_]Register{ .rax, .rcx, .rdx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; +/// TODO should the return_regs be in this array? +pub const callee_preserved_regs = [_]Register{ .rcx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; pub const c_abi_int_param_regs = [_]Register{ .rdi, .rsi, .rdx, .rcx, .r8, .r9 }; pub const c_abi_int_return_regs = [_]Register{ .rax, .rdx }; From 8fc3a707a44368f3f5c0ded0cc1b7080bf056761 Mon Sep 17 00:00:00 2001 From: Jacob G-W Date: Fri, 12 Nov 2021 20:00:59 -0500 Subject: [PATCH 2/4] x86_64/Emit: implement restoring callee_preserved_registers --- src/arch/x86_64/CodeGen.zig | 23 +++++++++++++++++++++++ src/arch/x86_64/Emit.zig | 36 ++++++++++++++++++++++++++++++++++++ src/arch/x86_64/Mir.zig | 17 ++++++++++++++++- 3 files changed, 75 insertions(+), 1 deletion(-) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index 2de2796983..0f060771e8 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -349,6 +349,13 @@ pub fn addExtraAssumeCapacity(self: *Self, extra: anytype) u32 { fn gen(self: *Self) InnerError!void { const cc = self.fn_type.fnCallingConvention(); if (cc != .Naked) { + // push the callee_preserved_regs that were used + const backpatch_push_callee_preserved_regs_i = try self.addInst(.{ + .tag = .push_regs_from_callee_preserved_regs, + .ops = undefined, + .data = .{ .regs_to_push_or_pop = undefined }, // to be backpatched + }); + _ = try self.addInst(.{ .tag = .push, .ops = (Mir.Ops{ @@ -423,6 +430,22 @@ fn gen(self: *Self) InnerError!void { }).encode(), .data = undefined, }); + // calculate the data for callee_preserved_regs to be pushed and popped + var callee_preserved_regs_push_data: u32 = 0x0; + inline for (callee_preserved_regs) |reg, i| { + if (self.register_manager.isRegAllocated(reg)) { + callee_preserved_regs_push_data |= 1 << @intCast(u5, i); + } + } + const data = self.mir_instructions.items(.data); + // backpatch the push instruction + data[backpatch_push_callee_preserved_regs_i].regs_to_push_or_pop = callee_preserved_regs_push_data; + // pop the callee_preserved_regs + _ = try self.addInst(.{ + .tag = .pop_regs_from_callee_preserved_regs, + .ops = undefined, + .data = .{ .regs_to_push_or_pop = callee_preserved_regs_push_data }, + }); _ = try self.addInst(.{ .tag = .ret, .ops = (Mir.Ops{ diff --git a/src/arch/x86_64/Emit.zig b/src/arch/x86_64/Emit.zig index 7430b8065b..7fd81d46c2 100644 --- a/src/arch/x86_64/Emit.zig +++ b/src/arch/x86_64/Emit.zig @@ -142,6 +142,9 @@ pub fn emitMir(emit: *Emit) InnerError!void { .dbg_epilogue_begin => try emit.mirDbgEpilogueBegin(inst), .arg_dbg_info => try emit.mirArgDbgInfo(inst), + .push_regs_from_callee_preserved_regs => try emit.mirPushPopRegsFromCalleePreservedRegs(.push, inst), + .pop_regs_from_callee_preserved_regs => try emit.mirPushPopRegsFromCalleePreservedRegs(.pop, inst), + else => { return emit.fail("Implement MIR->Isel lowering for x86_64 for pseudo-inst: {s}", .{tag}); }, @@ -244,6 +247,39 @@ fn mirPushPop(emit: *Emit, tag: Mir.Inst.Tag, inst: Mir.Inst.Index) InnerError!v 0b11 => unreachable, } } +fn mirPushPopRegsFromCalleePreservedRegs(emit: *Emit, tag: Mir.Inst.Tag, inst: Mir.Inst.Index) InnerError!void { + const callee_preserved_regs = bits.callee_preserved_regs; + // PUSH/POP reg + const opc: u8 = switch (tag) { + .push => 0x50, + .pop => 0x58, + else => unreachable, + }; + + const regs = emit.mir.instructions.items(.data)[inst].regs_to_push_or_pop; + if (tag == .push) { + for (callee_preserved_regs) |reg, i| { + if ((regs >> @intCast(u5, i)) & 1 == 0) continue; + const encoder = try Encoder.init(emit.code, 2); + encoder.rex(.{ + .b = reg.isExtended(), + }); + encoder.opcode_withReg(opc, reg.lowId()); + } + } else { + // pop in the reverse direction + var i = callee_preserved_regs.len; + while (i > 0) : (i -= 1) { + const reg = callee_preserved_regs[i - 1]; + if ((regs >> @intCast(u5, i - 1)) & 1 == 0) continue; + const encoder = try Encoder.init(emit.code, 2); + encoder.rex(.{ + .b = reg.isExtended(), + }); + encoder.opcode_withReg(opc, reg.lowId()); + } + } +} fn mirJmpCall(emit: *Emit, tag: Mir.Inst.Tag, inst: Mir.Inst.Index) InnerError!void { const ops = Mir.Ops.decode(emit.mir.instructions.items(.ops)[inst]); diff --git a/src/arch/x86_64/Mir.zig b/src/arch/x86_64/Mir.zig index 0d83bfed7c..05d374a856 100644 --- a/src/arch/x86_64/Mir.zig +++ b/src/arch/x86_64/Mir.zig @@ -264,8 +264,21 @@ pub const Inst = struct { /// arg debug info arg_dbg_info, - }; + /// push registers from the callee_preserved_regs + /// data is the bitfield of which regs to push + /// for example on x86_64, the callee_preserved_regs are [_]Register{ .rcx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; }; + /// so to push rcx and r8 one would make data 0b00000000_00000000_00000000_00001001 (the first and fourth bits are set) + /// ops is unused + push_regs_from_callee_preserved_regs, + + /// pop registers from the callee_preserved_regs + /// data is the bitfield of which regs to pop + /// for example on x86_64, the callee_preserved_regs are [_]Register{ .rcx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; }; + /// so to pop rcx and r8 one would make data 0b00000000_00000000_00000000_00001001 (the first and fourth bits are set) + /// ops is unused + pop_regs_from_callee_preserved_regs, + }; /// The position of an MIR instruction within the `Mir` instructions array. pub const Index = u32; @@ -284,6 +297,8 @@ pub const Inst = struct { got_entry: u32, /// Index into `extra`. Meaning of what can be found there is context-dependent. payload: u32, + /// A bitfield of which callee_preserved_regs to push + regs_to_push_or_pop: u32, }; // Make sure we don't accidentally make instructions bigger than expected. From 149bc79486907745c8ffb12a69f65189b26c6dbe Mon Sep 17 00:00:00 2001 From: Jacob G-W Date: Sat, 13 Nov 2021 12:52:17 -0500 Subject: [PATCH 3/4] add tests for previous commit --- src/link/Elf.zig | 2 +- test/cases.zig | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/link/Elf.zig b/src/link/Elf.zig index 9cad77d427..01b36b322f 100644 --- a/src/link/Elf.zig +++ b/src/link/Elf.zig @@ -2604,7 +2604,7 @@ fn addDbgInfoType(self: *Elf, ty: Type, dbg_info_buffer: *std.ArrayList(u8)) !vo // DW.AT.name, DW.FORM.string try dbg_info_buffer.writer().print("{}\x00", .{ty}); } else { - log.err("TODO implement .debug_info for type '{}'", .{ty}); + log.warn("TODO implement .debug_info for type '{}'", .{ty}); try dbg_info_buffer.append(abbrev_pad1); } }, diff --git a/test/cases.zig b/test/cases.zig index 3a8389f7d4..eb0b8fb8b9 100644 --- a/test/cases.zig +++ b/test/cases.zig @@ -1819,4 +1819,36 @@ pub fn addCases(ctx: *TestContext) !void { ":2:28: error: cannot set address space of local variable 'foo'", }); } + { + var case = ctx.exe("issue 10138: callee preserved regs working", linux_x64); + case.addCompareOutput( + \\pub fn main() void { + \\ const fd = open(); + \\ _ = write(fd, "a", 1); + \\ _ = close(fd); + \\} + \\ + \\fn open() usize { + \\ return 42; + \\} + \\ + \\fn write(fd: usize, a: [*]const u8, len: usize) usize { + \\ return syscall4(.WRITE, fd, @ptrToInt(a), len); + \\} + \\ + \\fn syscall4(n: enum { WRITE }, a: usize, b: usize, c: usize) usize { + \\ _ = n; + \\ _ = a; + \\ _ = b; + \\ _ = c; + \\ return 23; + \\} + \\ + \\fn close(fd: usize) usize { + \\ if (fd != 42) + \\ unreachable; + \\ return 0; + \\} + , ""); + } } From bc59a630ab7814ac0ca5487c780a03d7492648f7 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Fri, 19 Nov 2021 20:01:35 +0100 Subject: [PATCH 4/4] stage2,x86_64: fix genBinMathOp and clarify callee-saved regs Previously, we have confused callee-saved with caller-saved registers (the actual register sets were swapped). This commit fixes that for both `.x86` and `.x86_64` native backends. This commit also fixes the register allocation logic in `genBinMathOp` for `.x86_64` native backend where in a situation such that we require to spill a register, we would end up spilling the register that is already involved in the instruction as the other operand. In such a case, we make a note of this and spill a subsequent register instead. --- src/arch/x86/bits.zig | 14 +++++++------ src/arch/x86_64/CodeGen.zig | 41 ++++++++++++++++++++++++++++++------- src/arch/x86_64/bits.zig | 24 +++++++++++++--------- src/link/Elf.zig | 4 ++-- 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/src/arch/x86/bits.zig b/src/arch/x86/bits.zig index 5b981b9ef4..8231908956 100644 --- a/src/arch/x86/bits.zig +++ b/src/arch/x86/bits.zig @@ -32,11 +32,9 @@ pub const Register = enum(u8) { /// Returns the index into `callee_preserved_regs`. pub fn allocIndex(self: Register) ?u4 { return switch (self) { - .eax, .ax, .al => 0, - .ecx, .cx, .cl => 1, - .edx, .dx, .dl => 2, - .esi, .si => 3, - .edi, .di => 4, + .ebx, .bx, .bl => 0, + .esi, .si => 1, + .edi, .di => 2, else => null, }; } @@ -74,7 +72,11 @@ pub const Register = enum(u8) { // zig fmt: on -pub const callee_preserved_regs = [_]Register{ .eax, .ecx, .edx, .esi, .edi }; +/// These registers need to be preserved (saved on the stack) and restored by the callee before getting clobbered +/// and when the callee returns. +/// Note that .esp and .ebp also belong to this set, however, we never expect to use them +/// for anything else but stack offset tracking therefore we exclude them from this set. +pub const callee_preserved_regs = [_]Register{ .ebx, .esi, .edi }; // TODO add these to Register enum and corresponding dwarfLocOp // // Return Address register. This is stored in `0(%esp, "")` and is not a physical register. diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index 0f060771e8..8f3346e10b 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -159,6 +159,13 @@ pub const MCValue = union(enum) { => true, }; } + + fn isRegister(mcv: MCValue) bool { + return switch (mcv) { + .register => true, + else => false, + }; + } }; const Branch = struct { @@ -760,6 +767,19 @@ fn copyToNewRegister(self: *Self, reg_owner: Air.Inst.Index, mcv: MCValue) !MCVa return MCValue{ .register = reg }; } +/// Like `copyToNewRegister` but allows to specify a list of excluded registers which +/// will not be selected for allocation. This can be done via `exceptions` slice. +fn copyToNewRegisterWithExceptions( + self: *Self, + reg_owner: Air.Inst.Index, + mcv: MCValue, + exceptions: []const Register, +) !MCValue { + const reg = try self.register_manager.allocReg(reg_owner, exceptions); + try self.genSetReg(self.air.typeOfIndex(reg_owner), reg, mcv); + return MCValue{ .register = reg }; +} + fn airAlloc(self: *Self, inst: Air.Inst.Index) !void { const stack_offset = try self.allocMemPtr(inst); return self.finishAir(inst, .{ .ptr_stack_offset = stack_offset }, .{ .none, .none, .none }); @@ -1450,11 +1470,9 @@ fn genBinMathOp(self: *Self, inst: Air.Inst.Index, op_lhs: Air.Inst.Ref, op_rhs: // as the result MCValue. var dst_mcv: MCValue = undefined; var src_mcv: MCValue = undefined; - var src_inst: Air.Inst.Ref = undefined; if (self.reuseOperand(inst, op_lhs, 0, lhs)) { // LHS dies; use it as the destination. // Both operands cannot be memory. - src_inst = op_rhs; if (lhs.isMemory() and rhs.isMemory()) { dst_mcv = try self.copyToNewRegister(inst, lhs); src_mcv = rhs; @@ -1465,7 +1483,6 @@ fn genBinMathOp(self: *Self, inst: Air.Inst.Index, op_lhs: Air.Inst.Ref, op_rhs: } else if (self.reuseOperand(inst, op_rhs, 1, rhs)) { // RHS dies; use it as the destination. // Both operands cannot be memory. - src_inst = op_lhs; if (lhs.isMemory() and rhs.isMemory()) { dst_mcv = try self.copyToNewRegister(inst, rhs); src_mcv = lhs; @@ -1475,13 +1492,23 @@ fn genBinMathOp(self: *Self, inst: Air.Inst.Index, op_lhs: Air.Inst.Ref, op_rhs: } } else { if (lhs.isMemory()) { - dst_mcv = try self.copyToNewRegister(inst, lhs); + dst_mcv = if (rhs.isRegister()) + // If the allocated register is the same as the rhs register, don't allocate that one + // and instead spill a subsequent one. Otherwise, this can result in a miscompilation + // in the presence of several binary operations performed in a single block. + try self.copyToNewRegisterWithExceptions(inst, lhs, &.{rhs.register}) + else + try self.copyToNewRegister(inst, lhs); src_mcv = rhs; - src_inst = op_rhs; } else { - dst_mcv = try self.copyToNewRegister(inst, rhs); + dst_mcv = if (lhs.isRegister()) + // If the allocated register is the same as the rhs register, don't allocate that one + // and instead spill a subsequent one. Otherwise, this can result in a miscompilation + // in the presence of several binary operations performed in a single block. + try self.copyToNewRegisterWithExceptions(inst, rhs, &.{lhs.register}) + else + try self.copyToNewRegister(inst, rhs); src_mcv = lhs; - src_inst = op_lhs; } } // This instruction supports only signed 32-bit immediates at most. If the immediate diff --git a/src/arch/x86_64/bits.zig b/src/arch/x86_64/bits.zig index 7b09dc59e7..73c3abc8db 100644 --- a/src/arch/x86_64/bits.zig +++ b/src/arch/x86_64/bits.zig @@ -84,13 +84,11 @@ pub const Register = enum(u7) { /// Returns the index into `callee_preserved_regs`. pub fn allocIndex(self: Register) ?u4 { return switch (self) { - .rcx, .ecx, .cx, .cl => 0, - .rsi, .esi, .si => 1, - .rdi, .edi, .di => 2, - .r8, .r8d, .r8w, .r8b => 3, - .r9, .r9d, .r9w, .r9b => 4, - .r10, .r10d, .r10w, .r10b => 5, - .r11, .r11d, .r11w, .r11b => 6, + .rbx, .ebx, .bx, .bl => 0, + .r12, .r12d, .r12w, .r12b => 1, + .r13, .r13d, .r13w, .r13b => 2, + .r14, .r14d, .r14w, .r14b => 3, + .r15, .r15d, .r15w, .r15b => 4, else => null, }; } @@ -142,9 +140,15 @@ pub const Register = enum(u7) { // zig fmt: on -/// These registers belong to the called function. -/// TODO should the return_regs be in this array? -pub const callee_preserved_regs = [_]Register{ .rcx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; +/// These registers need to be preserved (saved on the stack) and restored by the callee before getting clobbered +/// and when the callee returns. +/// Note that .rsp and .rbp also belong to this set, however, we never expect to use them +/// for anything else but stack offset tracking therefore we exclude them from this set. +pub const callee_preserved_regs = [_]Register{ .rbx, .r12, .r13, .r14, .r15 }; +/// These registers need to be preserved (saved on the stack) and restored by the caller before +/// the caller relinquishes control to a subroutine via call instruction (or similar). +/// In other words, these registers are free to use by the callee. +pub const caller_preserved_regs = [_]Register{ .rax, .rcx, .rdx, .rsi, .rdi, .r8, .r9, .r10, .r11 }; pub const c_abi_int_param_regs = [_]Register{ .rdi, .rsi, .rdx, .rcx, .r8, .r9 }; pub const c_abi_int_return_regs = [_]Register{ .rax, .rdx }; diff --git a/src/link/Elf.zig b/src/link/Elf.zig index 01b36b322f..da8d6001b0 100644 --- a/src/link/Elf.zig +++ b/src/link/Elf.zig @@ -2604,12 +2604,12 @@ fn addDbgInfoType(self: *Elf, ty: Type, dbg_info_buffer: *std.ArrayList(u8)) !vo // DW.AT.name, DW.FORM.string try dbg_info_buffer.writer().print("{}\x00", .{ty}); } else { - log.warn("TODO implement .debug_info for type '{}'", .{ty}); + log.debug("TODO implement .debug_info for type '{}'", .{ty}); try dbg_info_buffer.append(abbrev_pad1); } }, else => { - log.err("TODO implement .debug_info for type '{}'", .{ty}); + log.debug("TODO implement .debug_info for type '{}'", .{ty}); try dbg_info_buffer.append(abbrev_pad1); }, }