Skip to content

stage2: remove operand from return instruction #5951

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

stage2: remove operand from return instruction #5951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src-self-hosted/Module.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1240,7 +1240,7 @@ fn astGenAndAnalyzeDecl(self: *Module, decl: *Decl) !bool {
.return_type = return_type_inst,
.param_types = param_types,
}, .{});
_ = try astgen.addZIRUnOp(self, &fn_type_scope.base, fn_src, .@"return", fn_type_inst);
_ = try astgen.addZIRUnOp(self, &fn_type_scope.base, fn_src, .ret_value, fn_type_inst);

// We need the memory for the Type to go into the arena for the Decl
var decl_arena = std.heap.ArenaAllocator.init(self.gpa);
Expand Down Expand Up @@ -1309,7 +1309,7 @@ fn astGenAndAnalyzeDecl(self: *Module, decl: *Decl) !bool {
!gen_scope.instructions.items[gen_scope.instructions.items.len - 1].tag.isNoReturn()))
{
const src = tree.token_locs[body_block.rbrace].start;
_ = try astgen.addZIRNoOp(self, &gen_scope.base, src, .returnvoid);
_ = try astgen.addZIRNoOp(self, &gen_scope.base, src, .@"return");
Copy link
Member

@andrewrk andrewrk Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a "set return value to void" instruction, which would be necessary for the "expected T, found void" compile error (when one forgets to return a value). I think we should keep the existing returnvoid instruction as well as the "return with an operand" instruction, and what this branch can do is add an additional "set the return value" instruction and "return control flow assuming the return value has already been set".

Idea being that we can have IR instructions with overlapping responsibilities; it's easy to have them call the appropriate functions during semantic analysis. This is in the effort of lowering memory usage; adding another enum tag to ir.Inst.Tag is free, so we may as well minimize the data we are allocating for these instructions.

However another observation is that once we do defers, it would make sense to have one canonical "exit the function" path from any scope. I think we should actually remove the return control flow instruction.

So, here's my proposal:

  • Add set_ret_val instruction, which does no control flow.
  • Add set_ret_void instruction. Same as set_ret_val but the operand is assumed to be the void value. (This is simply to reduce memory usage)
  • Remove return and returnvoid instructions.

There would be no more return control flow logic. It would be implied at the end of the main outer block of a function, and early-return control flow would be managed with breaks. I think this would work well with how to codegen defer expressions. Note, however, that the return value would either be set with ret_ptr and writing through the pointer, or set_ret_val directly.

The astgen for return syntax will need to change, to use a combination of the "break" and "set_ret_val" instructions rather than emitting the now-deleted "return" control flow instruction.

}

const fn_zir = try gen_scope_arena.allocator.create(Fn.ZIR);
Expand Down
14 changes: 3 additions & 11 deletions src-self-hosted/astgen.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,18 +457,10 @@ fn ret(mod: *Module, scope: *Scope, cfe: *ast.Node.ControlFlowExpression) InnerE
const tree = scope.tree();
const src = tree.token_locs[cfe.ltoken].start;
if (cfe.getRHS()) |rhs_node| {
if (nodeMayNeedMemoryLocation(rhs_node)) {
const ret_ptr = try addZIRNoOp(mod, scope, src, .ret_ptr);
const operand = try expr(mod, scope, .{ .ptr = ret_ptr }, rhs_node);
return addZIRUnOp(mod, scope, src, .@"return", operand);
} else {
const fn_ret_ty = try addZIRNoOp(mod, scope, src, .ret_type);
const operand = try expr(mod, scope, .{ .ty = fn_ret_ty }, rhs_node);
return addZIRUnOp(mod, scope, src, .@"return", operand);
}
} else {
return addZIRNoOp(mod, scope, src, .returnvoid);
Comment on lines -460 to -470
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're going for here, and I like it. I was thinking along the same lines of removing the operand from the return instruction. However I think we still need a "Set the return value" instruction which will happen in the else branch here. For an explanation of why see the review comments in codegen.zig on the store instruction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider:

fn hello(cond: bool) T {
    return if (cond) foo() else bar();
}

Intended semantics are: the result location of hello is passed directly as the result location of foo and bar. In master branch, this is the case. However with these changes, foo and bar will each be given a temporary result location, which is then copied to hello's result location. This copy is incompatible with pinned memory, which is something we plan to support soon with #3803 and #2765.

I think the if (nodeMayNeedMemoryLocation logic should remain.

const result = try expr(mod, scope, .none, rhs_node);
_ = try addZIRUnOp(mod, scope, src, .ret_value, result);
}
return addZIRNoOp(mod, scope, src, .@"return");
}

fn identifier(mod: *Module, scope: *Scope, rl: ResultLoc, ident: *ast.Node.OneToken) InnerError!*zir.Inst {
Expand Down
18 changes: 7 additions & 11 deletions src-self-hosted/codegen.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,7 @@ fn Function(comptime arch: std.Target.Cpu.Arch) type {
.ptrtoint => return self.genPtrToInt(inst.castTag(.ptrtoint).?),
.ref => return self.genRef(inst.castTag(.ref).?),
.ret => return self.genRet(inst.castTag(.ret).?),
.retvoid => return self.genRetVoid(inst.castTag(.retvoid).?),
.ret_value => return self.genRetVal(inst.castTag(.ret_value).?),
.store => return self.genStore(inst.castTag(.store).?),
.sub => return self.genSub(inst.castTag(.sub).?),
.unreach => return MCValue{ .unreach = {} },
Expand Down Expand Up @@ -1044,9 +1044,7 @@ fn Function(comptime arch: std.Target.Cpu.Arch) type {
}
}

fn ret(self: *Self, src: usize, mcv: MCValue) !MCValue {
const ret_ty = self.fn_type.fnReturnType();
try self.setRegOrMem(src, ret_ty, self.ret_mcv, mcv);
fn genRet(self: *Self, inst: *ir.Inst.NoOp) !MCValue {
switch (arch) {
.i386 => {
try self.code.append(0xc3); // ret
Expand All @@ -1059,18 +1057,16 @@ fn Function(comptime arch: std.Target.Cpu.Arch) type {
self.code.items[self.code.items.len - 5] = 0xe9; // jmp rel32
try self.exitlude_jump_relocs.append(self.gpa, self.code.items.len - 4);
},
else => return self.fail(src, "TODO implement return for {}", .{self.target.cpu.arch}),
else => return self.fail(inst.base.src, "TODO implement return for {}", .{self.target.cpu.arch}),
}
return .unreach;
}

fn genRet(self: *Self, inst: *ir.Inst.UnOp) !MCValue {
fn genRetVal(self: *Self, inst: *ir.Inst.UnOp) !MCValue {
const operand = try self.resolveInst(inst.operand);
return self.ret(inst.base.src, operand);
}

fn genRetVoid(self: *Self, inst: *ir.Inst.NoOp) !MCValue {
return self.ret(inst.base.src, .none);
const ret_ty = self.fn_type.fnReturnType();
try self.setRegOrMem(inst.base.src, ret_ty, self.ret_mcv, operand);
return .unreach;
}

fn genCmp(self: *Self, inst: *ir.Inst.BinOp, op: math.CompareOperator) !MCValue {
Expand Down
7 changes: 1 addition & 6 deletions src-self-hosted/codegen/c.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,7 @@ fn genFn(file: *C, decl: *Decl) !void {
switch (inst.tag) {
.assembly => try genAsm(file, inst.castTag(.assembly).?, decl),
.call => try genCall(file, inst.castTag(.call).?, decl),
.ret => try genRet(file, inst.castTag(.ret).?, decl, tv.ty.fnReturnType()),
.retvoid => try file.main.writer().print("return;", .{}),
.ret => try file.main.writer().print("return;", .{}),
else => |e| return file.fail(decl.src(), "TODO implement C codegen for {}", .{e}),
}
}
Expand All @@ -105,10 +104,6 @@ fn genFn(file: *C, decl: *Decl) !void {
try writer.writeAll("}\n\n");
}

fn genRet(file: *C, inst: *Inst.UnOp, decl: *Decl, expected_return_type: Type) !void {
return file.fail(decl.src(), "TODO return {}", .{expected_return_type});
}

fn genCall(file: *C, inst: *Inst.Call, decl: *Decl) !void {
const writer = file.main.writer();
const header = file.header.writer();
Expand Down
6 changes: 3 additions & 3 deletions src-self-hosted/ir.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub const Inst = struct {
ptrtoint,
ref,
ret,
retvoid,
ret_value,
/// Write a value to a pointer. LHS is pointer, RHS is value.
store,
sub,
Expand All @@ -84,14 +84,13 @@ pub const Inst = struct {
pub fn Type(tag: Tag) type {
return switch (tag) {
.alloc,
.retvoid,
.unreach,
.arg,
.breakpoint,
.ret,
=> NoOp,

.ref,
.ret,
.bitcast,
.not,
.isnonnull,
Expand All @@ -100,6 +99,7 @@ pub const Inst = struct {
.floatcast,
.intcast,
.load,
.ret_value,
=> UnOp,

.add,
Expand Down
20 changes: 8 additions & 12 deletions src-self-hosted/zir.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,14 +171,12 @@ pub const Inst = struct {
/// the memory location is in the stack frame, local to the scope containing the
/// instruction.
ref,
/// Obtains a pointer to the return value.
ret_ptr,
/// Obtains the return type of the in-scope function.
ret_type,
/// Sends control flow back to the function's callee. Takes an operand as the return value.
/// Sets the return value.
ret_value,
/// Sends control flow back to the function's callee.
@"return",
/// Same as `return` but there is no operand; the operand is implicitly the void value.
returnvoid,
/// Integer shift-left. Zeroes are shifted in from the right hand side.
shl,
/// Integer shift-right. Arithmetic or logical depending on the signedness of the integer type.
Expand Down Expand Up @@ -211,17 +209,15 @@ pub const Inst = struct {
return switch (tag) {
.arg,
.breakpoint,
.returnvoid,
.alloc_inferred,
.ret_ptr,
.ret_type,
.unreach_nocheck,
.@"unreachable",
.@"return",
=> NoOp,

.boolnot,
.deref,
.@"return",
.isnull,
.isnonnull,
.ptrtoint,
Expand All @@ -234,6 +230,7 @@ pub const Inst = struct {
.typeof,
.single_const_ptr_type,
.single_mut_ptr_type,
.ret_value,
=> UnOp,

.add,
Expand Down Expand Up @@ -350,7 +347,7 @@ pub const Inst = struct {
.primitive,
.ptrtoint,
.ref,
.ret_ptr,
.ret_value,
.ret_type,
.shl,
.shr,
Expand All @@ -369,7 +366,6 @@ pub const Inst = struct {
.condbr,
.compileerror,
.@"return",
.returnvoid,
.unreach_nocheck,
.@"unreachable",
=> true,
Expand Down Expand Up @@ -1842,10 +1838,10 @@ const EmitZIR = struct {
.arg => try self.emitNoOp(inst.src, .arg),
.breakpoint => try self.emitNoOp(inst.src, .breakpoint),
.unreach => try self.emitNoOp(inst.src, .@"unreachable"),
.retvoid => try self.emitNoOp(inst.src, .returnvoid),
.ret => try self.emitNoOp(inst.src, .@"return"),

.ret_value => try self.emitUnOp(inst.src, new_body, inst.castTag(.ret_value).?, .ret_value),
.not => try self.emitUnOp(inst.src, new_body, inst.castTag(.not).?, .boolnot),
.ret => try self.emitUnOp(inst.src, new_body, inst.castTag(.ret).?, .@"return"),
.ptrtoint => try self.emitUnOp(inst.src, new_body, inst.castTag(.ptrtoint).?, .ptrtoint),
.isnull => try self.emitUnOp(inst.src, new_body, inst.castTag(.isnull).?, .isnull),
.isnonnull => try self.emitUnOp(inst.src, new_body, inst.castTag(.isnonnull).?, .isnonnull),
Expand Down
24 changes: 11 additions & 13 deletions src-self-hosted/zir_sema.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub fn analyzeInst(mod: *Module, scope: *Scope, old_inst: *zir.Inst) InnerError!
.ensure_result_used => return analyzeInstEnsureResultUsed(mod, scope, old_inst.castTag(.ensure_result_used).?),
.ensure_result_non_error => return analyzeInstEnsureResultNonError(mod, scope, old_inst.castTag(.ensure_result_non_error).?),
.ref => return analyzeInstRef(mod, scope, old_inst.castTag(.ref).?),
.ret_ptr => return analyzeInstRetPtr(mod, scope, old_inst.castTag(.ret_ptr).?),
.ret_value => return analyzeInstRetVal(mod, scope, old_inst.castTag(.ret_value).?),
.ret_type => return analyzeInstRetType(mod, scope, old_inst.castTag(.ret_type).?),
.single_const_ptr_type => return analyzeInstSingleConstPtrType(mod, scope, old_inst.castTag(.single_const_ptr_type).?),
.single_mut_ptr_type => return analyzeInstSingleMutPtrType(mod, scope, old_inst.castTag(.single_mut_ptr_type).?),
Expand All @@ -68,7 +68,6 @@ pub fn analyzeInst(mod: *Module, scope: *Scope, old_inst: *zir.Inst) InnerError!
.@"unreachable" => return analyzeInstUnreachable(mod, scope, old_inst.castTag(.@"unreachable").?),
.unreach_nocheck => return analyzeInstUnreachNoChk(mod, scope, old_inst.castTag(.unreach_nocheck).?),
.@"return" => return analyzeInstRet(mod, scope, old_inst.castTag(.@"return").?),
.returnvoid => return analyzeInstRetVoid(mod, scope, old_inst.castTag(.returnvoid).?),
.@"fn" => return analyzeInstFn(mod, scope, old_inst.castTag(.@"fn").?),
.@"export" => return analyzeInstExport(mod, scope, old_inst.castTag(.@"export").?),
.primitive => return analyzeInstPrimitive(mod, scope, old_inst.castTag(.primitive).?),
Expand Down Expand Up @@ -115,7 +114,7 @@ pub fn analyzeBody(mod: *Module, scope: *Scope, body: zir.Module.Body) !void {
pub fn analyzeBodyValueAsType(mod: *Module, block_scope: *Scope.Block, body: zir.Module.Body) !Type {
try analyzeBody(mod, &block_scope.base, body);
for (block_scope.instructions.items) |inst| {
if (inst.castTag(.ret)) |ret| {
if (inst.castTag(.ret_value)) |ret| {
const val = try mod.resolveConstValue(&block_scope.base, ret.operand);
return val.toType();
} else {
Expand Down Expand Up @@ -296,8 +295,13 @@ fn analyzeInstCoerceToPtrElem(mod: *Module, scope: *Scope, inst: *zir.Inst.Coerc
return mod.coerce(scope, ptr.ty.elemType(), operand);
}

fn analyzeInstRetPtr(mod: *Module, scope: *Scope, inst: *zir.Inst.NoOp) InnerError!*Inst {
return mod.fail(scope, inst.base.src, "TODO implement analyzeInstRetPtr", .{});
fn analyzeInstRetVal(mod: *Module, scope: *Scope, inst: *zir.Inst.UnOp) InnerError!*Inst {
const b = try mod.requireRuntimeBlock(scope, inst.base.src);
const operand = try resolveInst(mod, scope, inst.positionals.operand);

// TODO analyze this into a store to .ret_ptr in callconv(.Unspecified)
// if the value is larger than pointer sized
return mod.addUnOp(b, inst.base.src, Type.initTag(.noreturn), .ret_value, operand);
}

fn analyzeInstRef(mod: *Module, scope: *Scope, inst: *zir.Inst.UnOp) InnerError!*Inst {
Expand Down Expand Up @@ -1091,15 +1095,9 @@ fn analyzeInstUnreachable(mod: *Module, scope: *Scope, unreach: *zir.Inst.NoOp)
return mod.analyzeUnreach(scope, unreach.base.src);
}

fn analyzeInstRet(mod: *Module, scope: *Scope, inst: *zir.Inst.UnOp) InnerError!*Inst {
const operand = try resolveInst(mod, scope, inst.positionals.operand);
const b = try mod.requireRuntimeBlock(scope, inst.base.src);
return mod.addUnOp(b, inst.base.src, Type.initTag(.noreturn), .ret, operand);
}

fn analyzeInstRetVoid(mod: *Module, scope: *Scope, inst: *zir.Inst.NoOp) InnerError!*Inst {
fn analyzeInstRet(mod: *Module, scope: *Scope, inst: *zir.Inst.NoOp) InnerError!*Inst {
const b = try mod.requireRuntimeBlock(scope, inst.base.src);
return mod.addNoOp(b, inst.base.src, Type.initTag(.noreturn), .retvoid);
return try mod.addNoOp(b, inst.base.src, Type.initTag(.noreturn), .ret);
}

fn floatOpAllowed(tag: zir.Inst.Tag) bool {
Expand Down
Loading