Skip to content

optimize udivmod - #15265

Merged
andrewrk merged 2 commits into
ziglang:masterfrom
tiehuis:optimize-udivmod
Jun 17, 2023
Merged

optimize udivmod#15265
andrewrk merged 2 commits into
ziglang:masterfrom
tiehuis:optimize-udivmod

Conversation

@tiehuis

@tiehuis tiehuis commented Apr 13, 2023

Copy link
Copy Markdown
Member

See https://reviews.llvm.org/D81809 for upstream description.

In summary this is ~10x improvement for small divisors and similar performance for equal divisors.

Closes #13523.


As a sanity, check, the below benchmark reinforces the visibly slow latency for small divisors:

const std = @import("std");
const Timer = std.time.Timer;

var prng = std.rand.DefaultPrng.init(0);
const random = prng.random();

const loops = 20_000_000;

pub fn main() !void {
    {
        var timer = try Timer.start();
        const start = timer.lap();
        var i: usize = 0;
        while (i < loops) : (i += 1) {
            const a = random.int(u128);
            const b = (random.int(u128) & 0xfff) + 1;

            const s = a / b;
            const r = a % b;
            std.mem.doNotOptimizeAway(s);
            std.mem.doNotOptimizeAway(r);
        }
        const end = timer.read();
        const ns_per_iteration = @intToFloat(f64, end - start) / loops;
        std.debug.print("udivmod: {d:.2}ns per\n", .{ns_per_iteration});
    }
}
# current master
$ zig build-exe -O ReleaseFast main.zig
$ ./main
udivmod: 215.85ns per

# this pr
$ zig-udivmod build-exe -O ReleaseFast main.zig
$ ./main 
udivmod: 24.17ns per

Some additional reading for those interested: https://danlark.org/2020/06/14/128-bit-division/

@setRuntimeSafety(is_test);
if (T == u64 and builtin.target.cpu.arch == .x86_64) {
var rem: T = undefined;
const quo = asm (

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This nets 10% perf improvement over the generic for x86. There are likely similar instructions for other architectures but don't have available for testing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Additionally this could be used for udivmoddi and the 64-bit by 32-bit divisions but have left these to take the generic path since I have not tested these.

const quo = asm (
\\divq %[v]
: [_] "={rax}" (-> T),
[_] "={rdx}" (rem),

@tiehuis tiehuis Apr 13, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This ideally would be [_] "={rdx}" (*r) and we wouldn't need the temporary, however the assembly syntax seems subtly different so this valid construct in C inline asm is not valid in zig.

@tiehuis

tiehuis commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

CI failure unrelated. Will rerun when the c main function definition is fixed.

@matu3ba matu3ba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As I understand it, divwide_generic assumes register size of 64 bit, which should be added as assumption and might create suboptimal code for those platforms.
See https://electronics.stackexchange.com/questions/499329/physical-size-of-the-registers-for-x86 for context.

Comment thread lib/compiler_rt/udivmod.zig Outdated
Comment thread lib/compiler_rt/udivmod.zig Outdated
Comment thread lib/compiler_rt/udivmod.zig
Comment thread lib/compiler_rt/udivmod.zig
Comment thread lib/compiler_rt/udivmod.zig Outdated
@matu3ba

matu3ba commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Windows fails due to inline asm blocks not being supported for bootstrapping with msvc:

compiler_rt-x86_64-windows-msvc.c(35976): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35976): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35976): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35976): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35977): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35977): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35977): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35977): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35978): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35978): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35978): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35978): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35978): warning C4047: '=': 'char [4]' differs in levels of indirection from 'const uint64_t'
compiler_rt-x86_64-windows-msvc.c(35978): error C2106: '=': left operand must be l-value
compiler_rt-x86_64-windows-msvc.c(35979): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35979): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35979): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35979): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35979): warning C4047: '=': 'char [4]' differs in levels of indirection from 'const uint64_t'
compiler_rt-x86_64-windows-msvc.c(35979): error C2106: '=': left operand must be l-value
compiler_rt-x86_64-windows-msvc.c(35980): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35980): error C2143: syntax error: missing ')' before ':'
compiler_rt-x86_64-windows-msvc.c(35981): warning C4047: '=': 'uint64_t' differs in levels of indirection from 'int (__cdecl *)()'
compiler_rt-x86_64-windows-msvc.c(35982): warning C4047: '=': 'uint64_t' differs in levels of indirection from 'int (__cdecl *)()'

Not sure, what the best workaround is.

@tiehuis

tiehuis commented Apr 13, 2023

Copy link
Copy Markdown
Member Author

As I understand it, divwide_generic assumes register size of 64 bit, which should be added as assumption and might create suboptimal code for those platforms. See https://electronics.stackexchange.com/questions/499329/physical-size-of-the-registers-for-x86 for context.

This should still work for 32-bit and the __udivmoddi4 tests should be exercising this generic path already:

zig/lib/compiler_rt/int.zig

Lines 103 to 105 in e2fe190

pub fn __udivmoddi4(a: u64, b: u64, maybe_rem: ?*u64) callconv(.C) u64 {
return udivmod(u64, a, b, maybe_rem);
}

You are right in that it probably isn't optimal but it shouldn't be worse than the current implementation.

Windows fails due to inline asm blocks not being supported for bootstrapping with msvc:

compiler_rt-x86_64-windows-msvc.c(35976): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35976): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35976): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35976): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35977): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35977): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35977): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35977): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35978): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35978): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35978): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35978): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35978): warning C4047: '=': 'char [4]' differs in levels of indirection from 'const uint64_t'
compiler_rt-x86_64-windows-msvc.c(35978): error C2106: '=': left operand must be l-value
compiler_rt-x86_64-windows-msvc.c(35979): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35979): error C2143: syntax error: missing ')' before 'string'
compiler_rt-x86_64-windows-msvc.c(35979): error C2143: syntax error: missing ';' before 'string'
compiler_rt-x86_64-windows-msvc.c(35979): error C2059: syntax error: ')'
compiler_rt-x86_64-windows-msvc.c(35979): warning C4047: '=': 'char [4]' differs in levels of indirection from 'const uint64_t'
compiler_rt-x86_64-windows-msvc.c(35979): error C2106: '=': left operand must be l-value
compiler_rt-x86_64-windows-msvc.c(35980): error C4235: nonstandard extension used: '__asm' keyword not supported on this architecture
compiler_rt-x86_64-windows-msvc.c(35980): error C2143: syntax error: missing ')' before ':'
compiler_rt-x86_64-windows-msvc.c(35981): warning C4047: '=': 'uint64_t' differs in levels of indirection from 'int (__cdecl *)()'
compiler_rt-x86_64-windows-msvc.c(35982): warning C4047: '=': 'uint64_t' differs in levels of indirection from 'int (__cdecl *)()'

Not sure, what the best workaround is.

Not sure on this either but if no suggestions on an easy fix I'll disable when the target is windows temporarily.

tiehuis added 2 commits June 17, 2023 13:50
See https://reviews.llvm.org/D81809 for upstream description.

In summary this is ~10x improvement for small divisors and similar
performance for equal divisors.

Closes ziglang#13523.
@tiehuis
tiehuis force-pushed the optimize-udivmod branch from a950c04 to 59a6f48 Compare June 17, 2023 02:32
@andrewrk

Copy link
Copy Markdown
Member

Great work!

@andrewrk
andrewrk merged commit 8d0a8c2 into ziglang:master Jun 17, 2023
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.

Poor performance of udivmod in zig compiler_rt

3 participants