Commit bc59a630ab

Jakub Konka <kubkon@jakubkonka.com>
2021-11-19 20:01:35
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.
1 parent 149bc79
Changed files (4)
src
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.
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 };
 
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
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);
         },
     }