Add big.Rational type to std #2102

tiehuis · 2019-03-26T07:00:01Z

Self-explanatory. I've made changes to Int to now have the concept of a read-only, constant type. This is currently intended primarily for internal usage but could be used externally. Ideally we should still use a fixed-buffer approach but this is infallible and never fails which is nice. A read-only Int is one which has a null allocator. I've made the allocator null so we can catch invalid writes to these types at runtime and panic if needed.

One note I've found when implementing the rational type is that there is an error in the Int division code. Using a Limb size of u8 or u16 will cause the tests to fail. However, since this is an existing problem in the Int code I've decided to make this PR now anyway. I'll reduce this test-case and make a separate issue for this in the next few days.

tiehuis · 2019-03-26T09:11:13Z

Interesting that the windows build failed. Will look closer tomorrow.

andrewrk · 2019-03-26T17:29:09Z

Hmm I wonder why stack traces aren't working. That error log isn't particularly helpful.

andrewrk · 2019-03-26T17:37:34Z

I opened it up in MSVC debugger and it seems to be an issue with compiler-rt:

pub extern fn __muloti4_windows_x86_64(a: *const i128, b: *const i128, overflow: *c_int) void {
00007FF6B4FEFFD0  push        rbp  
00007FF6B4FEFFD1  sub         rsp,50h  
00007FF6B4FEFFD5  lea         rbp,[rsp+50h]  
00007FF6B4FEFFDA  mov         qword ptr [rbp-8],rcx  
00007FF6B4FEFFDE  mov         qword ptr [rbp-10h],rdx  
00007FF6B4FEFFE2  mov         qword ptr [rbp-18h],r8  
    @setRuntimeSafety(builtin.is_test);
    compiler_rt.setXmm0(i128, __muloti4(a.*, b.*, overflow));
00007FF6B4FEFFE6  mov         rcx,qword ptr [a]  
=>00007FF6B4FEFFEA  mov         rdx,qword ptr [rcx]  
00007FF6B4FEFFED  mov         rcx,qword ptr [rcx+8]  
00007FF6B4FEFFF1  mov         r8,qword ptr [rbp-10h]  
00007FF6B4FEFFF5  mov         rax,qword ptr [r8]  
00007FF6B4FEFFF8  mov         r9,qword ptr [r8+8]  
00007FF6B4FEFFFC  mov         r8,qword ptr [rbp-18h]  
00007FF6B4FF0000  mov         r10,rsp  
00007FF6B4FF0003  mov         qword ptr [r10+20h],r8  
00007FF6B4FF0007  mov         qword ptr [rbp-20h],rcx  
00007FF6B4FF000B  mov         rcx,rdx  
00007FF6B4FF000E  mov         rdx,qword ptr [rbp-20h]  
00007FF6B4FF0012  mov         r8,rax  
00007FF6B4FF0015  call        compiler_rt.muloti4.__muloti4 (07FF6B4FF0030h)  
00007FF6B4FF001A  mov         rcx,rax  
00007FF6B4FF001D  call        setXmm0 (07FF6B4FEFE90h)  
}

stack trace:

>	test.exe!math.big.rational.gcdLehmer(builtin.StackTrace * r, math.big.int.Int *) Line 506	C
 	test.exe!math.big.rational.gcd(builtin.StackTrace * rma, math.big.int.Int *) Line 443	C
 	test.exe!math.big.rational.big.rational gcd large multi-limb result(builtin.StackTrace *) Line 594	C
 	test.exe!std.special.main(builtin.StackTrace *) Line 13	C

andrewrk · 2019-03-26T17:40:15Z

Can't wait to make some fractals with this :D

tiehuis · 2019-03-26T19:58:39Z

std/math/big/rational.zig

+//
+// r = gcd(x, y) where x, y > 0
+fn gcdLehmer(r: *Int, xa: Int, ya: Int) !void {
+    debug.assert(xa.positive and ya.positive);


Change all debug.assert's to use the testing module.

But this isn't in a test

/// This function is intended to be used only in tests. When `ok` is false, the test fails. /// A message is printed to stderr and then abort is called. pub fn expect(ok: bool) void {

Yes, correct. I made this note on the wrong assert. Latest commit is updated to use testing in appropriate places.

tiehuis · 2019-03-26T19:59:36Z

std/math/big/rational.zig

+const SignedDoubleLimb = @IntType(true, DoubleLimb.bit_count);
+
+fn gcd(rma: *Int, x: Int, y: Int) !void {
+    var r = rma;


Assert that rma is writable here.

andrewrk · 2019-03-26T20:02:01Z

std/math/big/rational.zig

+
+        for (str) |c, i| {
+            switch (state) {
+                State.Integer => {


If you like you can try out the new enum literal syntax here:

Suggested change

State.Integer => {

.Integer => {

andrewrk · 2019-03-26T20:02:35Z

std/math/big/rational.zig

+                State.Integer => {
+                    switch (c) {
+                        '.' => {
+                            state = State.Fractional;


it would work here too:

Suggested change

state = State.Fractional;

state = .Fractional;

tiehuis · 2019-03-27T09:33:31Z

Failing Int div case for 64-bit limbs (should fail on smaller limbs as well):

test "big.int zero-limb underflow (3.3 !positive branch)" {
    var a = try Int.initSet(al, 0x60000000000000000000000000000000000000000000000000000000000000000);
    var b = try Int.initSet(al, 0x10000000000000000);

    var q = try Int.init(al);
    var r = try Int.init(al);
    try Int.divTrunc(&q, &r, a, b);

    var expected = try Int.initSet(al, 0x6000000000000000000000000000000000000000000000000);
    testing.expect(q.eq(expected));
    testing.expect(r.eqZero());
}

Might do a fix in this PR. I'll probably cross-reference against the description in Knuth, since it is more widely used and might be an edge case not handled in the current.

daurnimator · 2019-03-27T11:46:10Z

std/math/big/int.zig

    }

-    pub fn isOdd(r: Int) bool {
-        return r.limbs[0] & 1 != 0;
+    pub fn isOdd(self: Int) bool {


Shouldn't this (and following methods) take a pointer to an Int?

Why? isOdd doesn't modify self and you should only pass a const pointer when you need the semantics of a pointer. I don't see the need for at pointer here.

Oh. I forget that arguments are const by default. Which I guess means that there won't be a copy?

Indeed :)
https://ziglang.org/documentation/master/#Pass-by-value-Parameters

Right. In my mind I had already decided that it would be cheaper to pass by reference.... from the zen:

Communicate intent precisely.

"Communicate intent precisely" also means don't communicate more than you intend to. Don't communicate that it needs to be a reference, if you don't need reference semantics.

daurnimator · 2019-03-27T12:36:11Z

std/math/big/rational.zig

+        r.q.swap(&other.q);
+    }
+
+    pub fn cmp(a: Rational, b: Rational) !i8 {


should take a pointer again?

tiehuis · 2019-03-28T07:42:00Z

Should have covered the main issue with the division code. I still plan to fuzz the main operations against gmp and cover any other issues soon.

andrewrk · 2019-03-28T19:53:03Z

@tiehuis do you want some help with the Windows compiler-rt issue? I don't know exactly what's wrong but maybe I can figure it out.

tiehuis · 2019-03-29T06:01:11Z

That would be helpful, thanks. I don't have a Windows setup to test on

tiehuis · 2019-04-03T04:27:27Z

Fixed all the main issues now with Int and also Rational. I've tested the core operations using this fuzzing code: https://gist.github.com/tiehuis/272ac675f1f48aea3fccc7fee1f7e9e9

There is a remaining issue in the setFloat code for smaller limbs, but presumably this is an incorrect assumption based on the Go code that was used for the port. That can be fixed later since we won't hit it (unless usize happened to be a u16 or less).

The remaining thing now is just the windows failure.

andrewrk · 2019-04-03T14:10:53Z

I'll look into the window failure today.

andrewrk · 2019-04-03T17:33:14Z

I wonder if this is related to #508.

tiehuis · 2019-04-09T05:49:31Z

Decided to pack the length/sign bits as I had been meaning to do for a while. Int is now 32-bits (down from 40-bits) and Rational is 64 bits (down from 80 bits).

Original discussion here: #1081 (comment)

andrewrk · 2019-04-11T02:44:30Z

I believe I have fixed the Windows compiler-rt issue in a4c7e4c.

I think your latest commit to this branch (774c200) had a regression which I'm guessing is easy to fix, and then once that's done this PR should be all tests passing and ready to go (after rebasing against master)!

A constant Int is one which has a value of null for its allocator field. It cannot be resized or have its limbs written. Any attempt made to write to it will be caught with a runtime panic.

The int div code still causes some edge cases to fail right now.

This effectively takes one-bit from the length field and uses it as the sign bit. It reduces the size of an Int from 40 bits to 32 bits on a 64-bit arch. This also reduces std.Rational from 80 bits to 64 bits.

tiehuis · 2019-04-11T07:38:05Z

Forgot to update the self-hosted compiler. Fixed and rebased against master.

tiehuis force-pushed the big.int-additions branch from f338a2f to 0a2e193 Compare March 26, 2019 07:12

tiehuis commented Mar 26, 2019

View reviewed changes

andrewrk reviewed Mar 26, 2019

View reviewed changes

daurnimator suggested changes Mar 27, 2019

View reviewed changes

andrewrk mentioned this pull request Apr 11, 2019

128 bit multiplication doesn't work on windows #2250

Closed

tiehuis added 7 commits April 11, 2019 19:36

Add read-only, non-allocating Int for internal constants

ea1d2a2

A constant Int is one which has a value of null for its allocator field. It cannot be resized or have its limbs written. Any attempt made to write to it will be caught with a runtime panic.

Add initial big.Rational type

5f4fcd5

Fix big.Int toString maybe-null allocator

b3ecdfd

Small fixes for big.Rational and corrections for gcdLehmer

d3e1f32

The int div code still causes some edge cases to fail right now.

Handle zero-limb trailing div case in big.Int

30788a9

Fix math.big.Int divN/gcdLehmer and fuzz-test failures

87d8ecd

Pack big.Int sign and length fields

78af62a

This effectively takes one-bit from the length field and uses it as the sign bit. It reduces the size of an Int from 40 bits to 32 bits on a 64-bit arch. This also reduces std.Rational from 80 bits to 64 bits.

tiehuis force-pushed the big.int-additions branch from 774c200 to 78af62a Compare April 11, 2019 07:37

tiehuis merged commit b59c65e into master Apr 11, 2019

tiehuis deleted the big.int-additions branch April 11, 2019 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add big.Rational type to std #2102

Add big.Rational type to std #2102

tiehuis commented Mar 26, 2019

tiehuis commented Mar 26, 2019

andrewrk commented Mar 26, 2019

andrewrk commented Mar 26, 2019

andrewrk commented Mar 26, 2019

tiehuis Mar 26, 2019

shawnl Mar 27, 2019

tiehuis Mar 27, 2019

tiehuis Mar 26, 2019

andrewrk Mar 26, 2019

andrewrk Mar 26, 2019

tiehuis commented Mar 27, 2019 •

edited

Loading

daurnimator Mar 27, 2019

Hejsil Mar 27, 2019

daurnimator Mar 27, 2019

Hejsil Mar 27, 2019

daurnimator Mar 27, 2019

andrewrk Mar 27, 2019

daurnimator Mar 27, 2019

tiehuis commented Mar 28, 2019

andrewrk commented Mar 28, 2019

tiehuis commented Mar 29, 2019

tiehuis commented Apr 3, 2019

andrewrk commented Apr 3, 2019

andrewrk commented Apr 3, 2019

tiehuis commented Apr 9, 2019

andrewrk commented Apr 11, 2019 •

edited

Loading

tiehuis commented Apr 11, 2019

Add big.Rational type to std #2102

Add big.Rational type to std #2102

Conversation

tiehuis commented Mar 26, 2019

tiehuis commented Mar 26, 2019

andrewrk commented Mar 26, 2019

andrewrk commented Mar 26, 2019

andrewrk commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiehuis commented Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiehuis commented Mar 28, 2019

andrewrk commented Mar 28, 2019

tiehuis commented Mar 29, 2019

tiehuis commented Apr 3, 2019

andrewrk commented Apr 3, 2019

andrewrk commented Apr 3, 2019

tiehuis commented Apr 9, 2019

andrewrk commented Apr 11, 2019 • edited Loading

tiehuis commented Apr 11, 2019

tiehuis commented Mar 27, 2019 •

edited

Loading

andrewrk commented Apr 11, 2019 •

edited

Loading