Skip to content

Add @divCeil builtin#16807

Closed
riverbl wants to merge 5 commits intoziglang:masterfrom
riverbl:div-ceil
Closed

Add @divCeil builtin#16807
riverbl wants to merge 5 commits intoziglang:masterfrom
riverbl:div-ceil

Conversation

@riverbl
Copy link
Contributor

@riverbl riverbl commented Aug 13, 2023

Closes #16022.

Includes implementation for comptime evaluation and lowering code for the LLVM, wasm and C backends. The C backend is untested (when I run zig build-exe -ofmt=c file.zig, the resulting C code does not have a main function and will not link).

Lowering code for all other backends has not been implemented.

Bigint ceiling division is implemented so that @divCeil works on comptime_int, but no method has been added to the std.math.big bigint type to expose this in the standard library.

There are existing duplicate tests in test/behavior/int_div.zig and test/behavior/math.zig. I've added new tests across both files.

@riverbl riverbl requested a review from kristoff-it as a code owner August 13, 2023 17:53
@riverbl riverbl marked this pull request as draft August 15, 2023 20:50
@riverbl
Copy link
Contributor Author

riverbl commented Aug 20, 2023

I've implemented @divCeil lowering code for the wasm backend, as there is a test for @divCeil on f16 enabled for the wasm backend, so it not being implemented was causing CI failure.

Currently, either signed division or equality comparison on integers of width smaller than 64 but not equal to 64 or 32 is broken for stage2 wasm. After division, the result is sign extended (incorrectly in the case of overflow) to either 32 or 64 bits, but the == operator expects its arguments to be 0 extended. This is the case for @divTrunc, and would be the case for @divFloor if it produced valid wasm.

When targeting wasm, the LLVM backend currently expects inputs to be sign extended, and ensures that the result is sign extended:

pub export fn div(x: i8, y: i8) i8 {
    return @divTrunc(x, y);
}

Gives (with the LLVM backend):

(func $div (type 0) (param i32 i32) (result i32)
    local.get 0
    local.get 1
    i32.div_s
    i32.extend8_s))

Note that inputs are assumed to already be sign extended and the result is sign extended (only necessary in the -128 / -1 case).

The stage2 wasm backend sign extends the arguments before performing the division, then doesn't do anything after the division, meaning the result will be sign extended if overflow didn't occur, and 0 extended if overflow did occur.

A consistent convention needs to be decided on for the stage2 wasm backend (@andrewrk please advise!) and either equality comparison or division changed.

My current @divCeil implementation ensures that the result is 0 extended.

Also, I haven't had any more feedback on unreachable vs @panic, so the new code contains some more @panics. I'm happy to recieve more feedback on this (@wooster0).

@riverbl riverbl marked this pull request as ready for review August 20, 2023 10:08
@ghost
Copy link

ghost commented Aug 20, 2023

Well I'm not a maintainer so I can't tell you what to do so I was more or less waiting for a maintainer to tell you the same thing (unless I'm wrong of course) but I believe this kind of thing where in a ReleaseFast build undefined behavior occurs with unexpected input is very much it works in Zig. This is what we write tests for. If the assumption is incorrect then you get a crash in a debug build of the program and UB in a ReleaseFast build.

It's similar with undefined:

In Debug mode, Zig writes 0xaa bytes to undefined memory. This is to catch bugs early

Debug mode is where we catch bugs. ReleaseFast is where we use unreachable assumptions for performance instead. It's the same with all the std.debug.asserts you see; they don't exist in ReleaseFast builds.

However, in ReleaseSafe mode, which is another optimize mode, you get good performance and safety so unreachable will indeed crash just like you're doing it with the explicit @panic.

And there is actually an accepted issue for changing master builds of the compiler to use ReleaseSafe instead of ReleaseFast (#15194) precisely because it just makes it easier to debug and report issues for users and contributors.
So if you change these kind of @panics to unreachable they will simply compile down to what is most appropriate for whatever optimize mode the user chooses.

I think @panic and std.debug.panic are more useful in scripts:

lib/std/Build.zig:                }) catch @panic("OOM");
lib/std/Build.zig:                }) catch @panic("OOM");
lib/std/Build.zig:                    }) catch @panic("OOM");
lib/std/Build.zig:                    }) catch @panic("OOM");
lib/std/Build.zig:                        .value = .{ .scalar = std.fmt.allocPrint(allocator, "{d}", .{v}) catch @panic("OOM") },
lib/std/Build.zig:                    }) catch @panic("OOM");
lib/std/Build.zig:            }) catch @panic("OOM");
lib/std/Build.zig:        ordered.append(OrderedUserInputOption.fromUnordered(allocator, entry.value_ptr.*)) catch @panic("OOM");
lib/std/Build.zig:            (self.build_root.join(self.allocator, &.{"zig-out"}) catch @panic("unhandled error"));

or if you implement debug functionality:

export fn __stack_chk_fail() callconv(.C) noreturn {
    @panic("stack smashing detected");
}

export fn __chk_fail() callconv(.C) noreturn {
    @panic("buffer overflow detected");
}

and just explicit debugging in general.

@riverbl
Copy link
Contributor Author

riverbl commented Aug 30, 2023

It looks like the previous CI failure was due to the change to langref.html.in - I suspect that the compiler used for checking the documentation doesn't include the newly added @divCeil intrinsic, so regarded the examples including it as erroneous. Documentation will have to be added in a later pull request to avoid such CI failures.

@ghost
Copy link

ghost commented Aug 30, 2023

Hmm I was able to add langref docs for the @trap builtin just fine when I added it: https://github.com/ziglang/zig/pull/14782/files#diff-9674f03a76728eff712dfbd966487142bf4103b4a58049ee9696028e890f86bcR9398
I'm pretty sure there isn't "a compiler checking the documentation" like that. The only check that's really done on the langref.html file AFAIK is this HTML Tidy check that you can find in some of the CI scripts:

# Look for HTML errors.
# TODO: move this to a build.zig flag (-Denable-tidy)
tidy --drop-empty-elements no -qe "../zig-out/doc/langref.html"

I would try running that locally and viewing the langref locally as well to spot any mistakes. It might be some unnecessary (leading/trailing) whitespace.

@mlugg
Copy link
Member

mlugg commented Aug 30, 2023

I'm pretty sure there isn't "a compiler checking the documentation" like that.

There is: docgen (which is run by zig test docs in the CI script) runs all the tests in the documentation and emits an error if they don't have the expected output. This is, however, done with the stage3 compiler, so it should include the new builtin. @riverbl, try running tools/docgen.zig over langref.html.in (with your new compiler) and see if it reports any errors.

@riverbl
Copy link
Contributor Author

riverbl commented Aug 31, 2023

try running tools/docgen.zig over langref.html.in (with your new compiler) and see if it reports any errors.

I just tried that (with the version of langref.html.in that was causing CI failure) and it builds the documentation page fine, without reporting any errors.

The switch in lib/docs/main.js:1633 doesn't handle @divCeil, but I don't see why that would cause CI failure only when langref.html.in is modified.

Includes implementation for comptime evaluation and lowering code for LLVM and C backends.
Add section on `@divCeil` to language reference

Replace some `@panic`s with `unreachable`s

Add test cases for `@divCeil` for the stage2 wasm backend

Fix breakage after rebase
Add `@divCeil` to binary operators list in documentation

Fix breakage after rebase
Fix execution reaching `unreachable` when calling `@divCeil` on `u128` or `i128` with x86_64 backend

Fix breakage after rebase
@riverbl
Copy link
Contributor Author

riverbl commented Jan 23, 2024

@andrewrk @kristoff-it Ready for review.

@andrewrk
Copy link
Member

andrewrk commented May 9, 2024

I'm sorry, I didn't review this in time, and now it has bitrotted. Furthermore, so many pull requests have stacked up that I can't keep up and I am therefore declaring Pull Request Bankruptcy and closing old PRs that now have conflicts with master branch.

If you want to reroll, you are by all means welcome to revisit this changeset with respect to the current state of master branch, and there's a decent chance your patch will be reviewed the second time around.

Either way, I'm closing this now, otherwise the PR queue will continue to grow indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: promote std.math.divCeil to @divCeil builtin

3 participants