ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
32.34k stars 2.36k forks source link

Proposal: Improve overflow operations #20203

Open Snektron opened 1 month ago

Snektron commented 1 month ago

Context

The current overflow operations (@addWithOverflow, @subWithOverflow, @mulWithOverflow, @shlWithOverflow) do currently not spark joy in my opinion: These seem initially just designed from the corresponding LLVM instruction, however, they have some problems in reality:

Proposal

My proposal is to change the overflow builtins to return the full overflow value too. The interface would largely remain the same, only the return types need to be modified:

Rexicon226 commented 1 month ago

I think that @shlWithOverflow should return struct{ @TypeOf(a), std.meta.Int(.unsigned, b) } instead.

In an example such as @shlWithOverflow(@as(u64, x), 32), the maximum amount the overflow could be is u32. This would remove a potential cast that needs to be done.

mlugg commented 1 month ago

Note that if we accept ranged ints (#3806), then @{add,sub,mul}WithOverflow become kind of redundant, and even more so under this proposal. So, if that's accepted, perhaps those builtins should be eliminated entirely, and implemented in userspace? e.g.

fn addWithOverflow(comptime T: type, a: T, b: T) struct { u1, T } {
    const result = a + b;
    const overflow = result < minInt(T) or result > maxInt(T);
    return .{ overflow, @truncate(result) };
}
Rexicon226 commented 1 month ago

I think that @shlWithOverflow should return struct{ @TypeOf(a), std.meta.Int(.unsigned, b) } instead.

In an example such as @shlWithOverflow(@as(u64, x), 32), the maximum amount the overflow could be is u32. This would remove a potential cast that needs to be done.

My brain woke up. b is not required to be comptime known, so this doesn't make sense. Proposal makes sense as is.

rohlem commented 1 month ago

@subWithOverflow [...] would work cleanly with returning an i1 instead of a u1: A carry returns -1 and no carry returns 0. In this case carries can be naively added to the next word [...] .

I was initially quite confused by this: I assume this was formulated only for unsigned numbers, right? Because f.e. @as(i2, -2) - @as(i2, 1) does overflow negatively, while @as(i2, 1) - @as(i2, -2) instead overflows positively. So for signed numbers an i1 doesn't seem more clear to me. To allow "naively adding" the result we would have to allow overflow of -1, 0, and 1, extending the result type to i2. (Which I think would be undesired because it's more work?) But maybe signed operations simply aren't as frequently used here, or I'm missing some other bit-fiddling insight that makes this a moot point.

EDIT: Actually, for the use case of "adding it to the next word", i.e. when implementing a big integer out of several segments/limbs, the sign information (I assume) won't be stored at each segment individually anyway. The more I think about it, the more I'd probably implement a - b as a + (-b), and individually add limbs as unsigned numbers, in which case the signed variants don't even apply. For other use cases of signed number subtraction, knowing the sign of the overflow might still be helpful I assume, though I'm not sure.

Snektron commented 1 month ago

I was initially quite confused by this: I assume this was formulated only for unsigned numbers, right? Because f.e. @as(i2, -2) - @as(i2, 1) does overflow negatively, while @as(i2, 1) - @as(i2, -2) instead overflows positively.

Yep, youre right, I didnt think about that. I think it would still make sense to return the complete carry, and both addWithOverflow and subWithOverflow would return {T, i2}.

Snektron commented 1 month ago

Note that if we accept ranged ints (https://github.com/ziglang/zig/issues/3806), then @{add,sub,mul}WithOverflow become kind of redundant, and even more so under this proposal.

It seems to me that if we want those operations, then it would still be useful to have a wide_mul AIR instruction rather than upcast-and-mul, so internally the proposed changes would still be useful imo.