https://godbolt.org/z/Yzas6K1aY
This is my first Zig code, I'd like a review of what I should be doing differently/better/etc.
#Code Review: Brainf*ck interpreter
upper sundial
pub const Inst = extern struct {
tag: Tag,
const Tag = enum(c_int) {
end,
increment,
decrement,
shift,
unshift,
jump,
dejump,
};
pub fn instIncrement(mem: [*]u32, dp: usize) void {
mem[dp] += 1;
}
pub fn instDecrement(mem: [*]u32, dp: usize) void {
mem[dp] -= 1;
}
pub fn instShift(dp: *usize) void {
dp.* += 1;
}
pub fn instUnshift(dp: *usize) void {
dp.* -= 1;
}
pub fn instJump(mem: [*]u32, dp: usize, code: [*]const Inst, ip: *usize) void {
if (mem[dp] != 0) return;
var depth: usize = 0;
while (true) : (ip.* += 1) {
const inst = code[ip.*];
switch (inst.tag) {
inline else => {},
inline .jump => depth += 1,
inline .dejump, .end => {
if (depth == 0) {
ip.* -= 1;
return;
} else depth -= 1;
},
}
}
}
pub fn instDejump(mem: [*]u32, dp: usize, code: [*]const Inst, ip: *usize) void {
if (mem[dp] == 0) return;
var depth: usize = 0;
while (true) : (ip.* -= 1) {
const inst = code[ip.*];
switch (inst.tag) {
inline else => {},
inline .dejump => depth += 1,
inline .jump, .end => {
if (depth == 0) {
ip.* -= 1;
return;
} else depth -= 1;
},
}
}
}
};
pub export fn run(mem: [*]u32, code: [*]const Inst, len: usize) void {
if (code[len - 1].tag != Inst.Tag.end) return;
var ip: usize = 0;
var dp: usize = 0;
while (true) : (ip += 1) {
const inst = code[ip];
switch (inst.tag) {
.end => return,
.increment => Inst.instIncrement(mem, dp),
.decrement => Inst.instDecrement(mem, dp),
.shift => Inst.instShift(&dp),
.unshift => Inst.instUnshift(&dp),
.jump => Inst.instJump(mem, dp, code, &ip),
.dejump => Inst.instDejump(mem, dp, code, &ip),
}
}
}
pub export fn main() void {
const memSize = 16;
var mem: [memSize]u32 = .{0} ** memSize;
const insns = [_]Inst{
.{ .tag = .increment },
.{ .tag = .increment },
.{ .tag = .increment },
.{ .tag = .shift },
.{ .tag = .increment },
.{ .tag = .increment },
.{ .tag = .unshift },
.{ .tag = .decrement },
.{ .tag = .end },
};
run(&mem, &insns, insns.len);
}
true cradle
do you actually need this to be exported? otherwise you could make much more efficient use of slices
true cradle
perhaps, I just don't think multiple item pointers are very good when you have bounded data here, for example the memory and instruction list
upper sundial
what do you mean by "not very good", what are we looking at? Worse performance? Idiomatic code?
export is not a requirement, but it's the only way I know to look at the assembly being output 😅
using Godbolt
true cradle
well it seems to me that you're writing very "export-oriented", for example using c_ints and not uses slices, so I'm not sure whether it's appropriate to critique it based on idiomatic code
but this is not very safe
upper sundial
right, but now the output is humongous with a bunch of POSIX stuff, lemme see if I can remove that
alright, that does work!
just have to name it something other than main
upper sundial
true cradle
but yeah try to use slices where you can, you get the safety and its "better zig"
upper sundial
I wrote it with slices, but then I had to get rid of them when working on Godbolt
true cradle
okay nice
upper sundial
this is much nicer!
true cradle
is ip instruction pointer?
upper sundial
yup
true cradle
and dp is data pointer
upper sundial
and dp is the data pointer yeah
true cradle
okay
upper sundial
sorry, not many comments, they got eaten while rewriting
true cradle
little bit over-zealous with the inlines btw
you don't need inlines there
I'm not sure whether that allows the compiler to optimise better or not (probably does), but just general review
upper sundial
yup, thanks
I went back and forth a bit there, because I'm using .jump, .end and I was wondering if that needed inline
true cradle
yeah you generally don't need inline in most instances
I think the only case is when you accept a payload and the types are different
true cradle
const Animal = union(enum) {
dog: Dog,
cat: Cat
};
...
switch (animal) {
inline .dog, .cat => |pet| pet.feed(),
}
depends what you're looking for code review but that's the only zig-specific things I can really spot
upper sundial
I know about the unsafety, that part is fine for now
I'd have to figure out what semantics are there and I wanted it simple for now, so it just doesn't check bounds right now
is there a "zig way" to deal with "slice plus pointer into slice"?
true cradle
maybe more less-zig-specific but I'm not sure what the point of the Inst class is, it seems like just a namespace? which is fine, but you can probably make use of more compact code here
upper sundial
I don't know, just me messing around to get things to export right
true cradle
for example you have a tag field in that class, but Zig has good syntax sugar that would let you do tag.process(mem, dp, ip) or something, in which case that could do
pub const Inst = enum {
end,
increment,
decrement,
shift,
unshift,
jump,
dejump,
pub fn process(self: Inst, mem: []u32, dp: *usize, ip: *usize) void {
switch (self.tag) {
.increment => mem[dp.*] += 1,
.decrement => mem[dp.*] -= 1,
.shift => etc.,
...
}
}
};
pub fn run(mem: []u32, code: []const Inst) void {
if (code[len - 1] != Inst.end) return;
var ip: usize = 0;
var dp: usize = 0;
for (code) |inst| inst.process();
}
I know that's a bit more of a drastic change
true cradle
ha yeah that's basically what I was suggesting
upper sundial
for example you have a `tag` field in that class, but Zig has good syntax sugar ...
would this still result in correct branch prediction? I'd have to try that
I was hoping to do something like that
I'll try that next now that my code is in much better shape 😊
true cradle
I imagine so, but yeah you'll have to try it. I don't come from a low level background so this isn't the sort of stuff I think about
upper sundial
me neither, this is my babby's first steps into low-level programming: interpreter loops
but I do know what it sorta needs to look like
I'm basing myself off the knowledge in this issue partly: https://github.com/ziglang/zig/issues/8220
true cradle
btw, shameless "plug" but I thought you might find this interesting if you're learning zig: https://github.com/edqx/ccb
upper sundial
I know that I need to avoid each prong of the switch jumping to the same spot
true cradle
I'm basing myself off the knowledge in this issue partly: <https://github.com/zi...
I see
upper sundial
btw, shameless "plug" but I thought you might find this interesting if you're le...
sweet!
I'll need to read that
I'd love to "upgrade" to a small Forth maybe, we'll see, no REPL though, since Zig can't JIT
true cradle
not familiar with that
do you mean this? https://en.wikipedia.org/wiki/Forth_(programming_language)#
const memSize = 16;
var mem: [memSize]u32 = .{0} ** memSize;
this is pretty heavily discouraged these days, especially if you have larger memory sizes
if you're only initialising once, it's better to just manually set it to 0 with @memset(mem, 0)
upper sundial
yeah, my initial version used arena allocation
but I couldn't get the resulting slice to work with Godbolt 🙃
what's the best "simple" allocator to use for something like that?
true cradle
what's the best "simple" allocator to use for something like that?
when in doubt, use general purpose allocator
or c_allocator if you want something faster
and any faster than that it's probably the wrong question to ask
but gpa is safe and warns you about memory leaks and double frees in debug mode, so it's a solid choice unless you're micro optimising
upper sundial
sweet
true cradle
don't think I have anything more to suggest for code review, other people might do though
true cradle
sounds good
upper sundial
pub export fn entry() void {
var gpa = @import("std").heap.GeneralPurposeAllocator(.{}){};
defer _ = gpa.deinit();
const allocator = gpa.allocator();
var mem = allocator.alloc(u32, 16) catch &[_]u32{};
const insns = [_]Inst{
.increment,
};
run(mem, &insns, insns.len);
}
it complains that I shouldn't use var for the mem variable
but run expects []u32
true cradle
yeah that's fine, const is for the memory the pointer points to, not for the variable itself
and slices are pointers in Zig, so const mem: []u32 = allocator.alloc(u32, 16)
upper sundial
no, it errors rn
true cradle
hm
upper sundial
the only "downside" of using GPA is that my code size explodes again heh
it goes from 100 to 1000 instructions if I use an allocator
true cradle
yeah it's the &[_]u32{};
const mem = allocator.alloc(u32, 16) catch blk: {
var empty = [_]u32{};
break :blk ∅
};
that works well
true cradle
it goes from 100 to 1000 instructions if I use an allocator
try c_allocator
nvm you need libc for that
-lc lol
upper sundial
I might go back to my earlier code since it compiles down to nothing 
haha
const mem: []u32 = allocator.alloc(u32, 16) catch &.{};
oooh this works
true cradle
ha gpa is pretty hefty
but I guess that's because it's a pure zig implementation
whereas c_allocator uses libc
true cradle
yeah
syscall
nope not a syscall
I don't know enough about low level programming :p
upper sundial
same
upper sundial
for example you have a `tag` field in that class, but Zig has good syntax sugar ...
I tested this and this does indeed stop the compiler from correctly compiling the interpreter loop
just as a fyi
fn step(self: *VM) void {
const inst = self.code[self.ip];
switch (inst) {
.end => return,
.increment => self.instIncrement(),
.decrement => self.instDecrement(),
.shift => self.instShift(),
.unshift => self.instUnshift(),
.jump => self.instJump(),
.dejump => self.instDejump(),
}
}
fn run(self: *VM) void {
while (true) : (self.ip += 1) {
self.step();
}
}
this does the wrong thing for example
fn run(self: *VM) void {
while (true) : (self.ip += 1) {
const inst = self.code[self.ip];
switch (inst) {
.end => return,
.increment => self.instIncrement(),
.decrement => self.instDecrement(),
.shift => self.instShift(),
.unshift => self.instUnshift(),
.jump => self.instJump(),
.dejump => self.instDejump(),
}
}
}
yet this does the right thing 🙂
true cradle
what if you inlined it?
upper sundial
still doesn't work for some reason
it's the increment of ip that is causing the issue I think
not sure, but in any case, you have to do it exactly like this or the code-gen creates a single jump point
upper sundial
upper sundial
what if you inlined it?
inlining the else does work
fn step(self: *VM, inst: Inst) void {
switch (inst) {
.increment => self.increment(),
.decrement => self.decrement(),
.shift => self.shift(),
.unshift => self.unshift(),
.jump => self.jump(),
.dejump => self.dejump(),
.end => {},
}
}
fn run(self: *VM) void {
while (true) switch (self.code[self.ip]) {
.end => return,
inline else => |inst| {
self.step(inst);
self.ip += 1;
},
};
}
this is kinda perfect