saturating arithmetic builtins: add, sub, mul, shl #9619

travisstaloch · 2021-08-24T23:45:55Z

adds 1 simple behavior tests for each
which does integer and vector ops at
runtime and comptime
adds bigint_*_sat() methods for each
which simply set dest to the type
min/max boundaries if they are
exceeded. these should to be
audited for correctness and perhaps
there is a better existing way to
accomplish this.
the mul intrinsic needs to somehow
explicitly provide a scale parameter
(which seems to default to 0?)

travisstaloch · 2021-08-24T23:51:34Z

addresses #1284

Snektron · 2021-08-25T00:13:21Z

Don't forget to update the langref

src/stage1/bigint.cpp

src/zig_llvm.cpp

andrewrk · 2021-08-25T05:09:52Z

Nice work! Did you want to implement the new syntax too? This syntax is planned:

a +| b - saturating addition
a -| b - saturating subtraction
a *| b - saturating multiplication
a <<| b - saturating shift left
a +|= b - saturating assignment addition
a -|= b - saturating assignment subtraction
a *|= b - saturating assignment multiplication
a <<|= b - saturating assignment shift left

If not, that's ok, since implementing them as builtins is an incremental improvement getting us closer to the final spec.

travisstaloch

If not, that's ok, since implementing them as builtins is an incremental improvement getting us closer to the final spec.

I am thinking the operator stuff should be a follow up pr to keep things a little more manageable.

src/Zir.zig

src/stage1/bigint.cpp

src/zig_llvm.cpp

src/stage1/bigint.cpp

travisstaloch · 2021-08-25T18:45:50Z

I'm not sure what is causing the ci failure. I'm thinking its likely related to the bigint changes. Perhaps failing on a 32bit arch? Just a guess as i can't tell from looking at the logs. Anyway looks like i'll need to rework my bigint changes.

matu3ba

please add some explanations on the logic.

Nits: a few interesting test cases and explaining the workaround with is_negative due to bigints_bits_needed implementation.

test/behavior/saturating_arithmetic.zig

src/stage1/bigint.cpp

andrewrk

Nice work! I left you a few items to address but I think this is very close to being mergeable.

src/stage1/ir.cpp

test/behavior/saturating_arithmetic.zig

src/Zir.zig

doc/langref.html.in

- adds 1 simple behavior tests for each which does integer and vector ops at runtime and comptime - adds bigint_*_sat() methods for each which simply set dest to the type min/max boundaries if they are exceeded. these should to be audited for correctness and perhaps there is a better existing way to accomplish this. - the mul intrinsic needs to somehow explicitly provide a scale parameter (which seems to default to 0?)

- rather than initializing new integers for min/max, compute the number of bits required using bigint_bits_needed(). NOTE: this doesn't allow for types larger than 64 bits. - rename bigint_saturate() -> bigint_clamp_by_bitcount() - adds a few more test cases

- use CreateIntrinsic() which accepts a variable number of arguments to pass the scale parameter

- rather than re-initializing, we only need to shift the least significant digit right by the number of 'extra' bits

- works for ints > 64 bits - add a few more test cases for ints > 64 bits

- move bound computations closer to usage within cases - rename op -> dest for consistency

- adds more test cases suggested by matu3ba - better explain clamping algorithm in comments

- rework test cases which were skipping runtime evaluation. this uncovered bugs related to *mul.fix.sat intrinsic. - removed extra type args in ZigLLVMBuild*MulFixSat so mul.fix.sat intrinsic is called correctly - replaced panics in ir.cpp with compile errors. - added case to test/compile_errors.zig given floats - updated langref: indcate future `a +| b` syntax

- account for `maxInt(iN) *| -1 == minInt(iN) + 1` - explain upstream bug in llvm.smul.fix.sat and link to ziglang#9643 in langref and commented out test cases

- ci is erroring with 'LLVM ERROR: Unable to expand fixed point multiplication' when compiling for wasm32 - just trying to get ci to pass. not sure this is correct to skip

- prevent the llvm error by making test helper param `op` comptime. this way mul instructions are only generated in mul test

Snektron reviewed Aug 25, 2021

View reviewed changes

src/stage1/bigint.cpp Outdated Show resolved Hide resolved

src/zig_llvm.cpp Outdated Show resolved Hide resolved

travisstaloch commented Aug 25, 2021

View reviewed changes

src/Zir.zig Outdated Show resolved Hide resolved

src/stage1/bigint.cpp Outdated Show resolved Hide resolved

src/zig_llvm.cpp Outdated Show resolved Hide resolved

src/stage1/bigint.cpp Outdated Show resolved Hide resolved

src/stage1/bigint.cpp Outdated Show resolved Hide resolved

travisstaloch mentioned this pull request Aug 26, 2021

get rid of llvm_intrinsics.zig travisstaloch/simdjzon#2

Open

10 tasks

matu3ba suggested changes Aug 28, 2021

View reviewed changes

andrewrk requested changes Aug 28, 2021

View reviewed changes

src/stage1/ir.cpp Outdated Show resolved Hide resolved

test/behavior/saturating_arithmetic.zig Outdated Show resolved Hide resolved

src/Zir.zig Outdated Show resolved Hide resolved

doc/langref.html.in Show resolved Hide resolved

travisstaloch mentioned this pull request Aug 28, 2021

@mulWithSaturation discovered upstream llvm bug #9643

Closed

travisstaloch force-pushed the sat-arithmetic branch from f0d4ba7 to 2475e9b Compare August 28, 2021 21:45

travisstaloch added 10 commits August 28, 2021 19:52

zig_llvm: pass scale = 0 to mul_fix_sat intrinsics

bf33759

- use CreateIntrinsic() which accepts a variable number of arguments to pass the scale parameter

update langref

b18d436

simplify bigint_clamp_by_bitcount()

1a4d70a

- rather than re-initializing, we only need to shift the least significant digit right by the number of 'extra' bits

rework bigint_clamp_by_bitcount() again

db389ce

- works for ints > 64 bits - add a few more test cases for ints > 64 bits

clean up bigint_clamp_by_bitcount()

0b74123

- move bound computations closer to usage within cases - rename op -> dest for consistency

sat-arithmetic: add more test cases

6b6c63a

- adds more test cases suggested by matu3ba - better explain clamping algorithm in comments

sat-arithmetic: add missing bigint deinit

437fba2

travisstaloch force-pushed the sat-arithmetic branch from 2475e9b to 6b21164 Compare August 29, 2021 02:58

travisstaloch added 4 commits August 30, 2021 16:48

sat-arithmetic: tweak bigint clamping

fc0ff06

- account for `maxInt(iN) *| -1 == minInt(iN) + 1` - explain upstream bug in llvm.smul.fix.sat and link to ziglang#9643 in langref and commented out test cases

Merge branch 'ziglang:master' into sat-arithmetic

039c35c

sat-arithmetic: skip mul tests if arch == .wasm32

4e11ae1

- ci is erroring with 'LLVM ERROR: Unable to expand fixed point multiplication' when compiling for wasm32 - just trying to get ci to pass. not sure this is correct to skip

travisstaloch force-pushed the sat-arithmetic branch from 78058bf to 4e11ae1 Compare August 31, 2021 02:19

travisstaloch added 2 commits August 30, 2021 21:35

sat-arithmetic: skip mul tests if arch == .wasm32

4880d87

- prevent the llvm error by making test helper param `op` comptime. this way mul instructions are only generated in mul test

sat-arithmetic: wasm shl - skip testing ints > 64 bits

938b5bf

travisstaloch force-pushed the sat-arithmetic branch from cf07a96 to 938b5bf Compare September 1, 2021 05:10

travisstaloch requested a review from andrewrk September 1, 2021 16:42

andrewrk merged commit 21a5769 into ziglang:master Sep 1, 2021

travisstaloch deleted the sat-arithmetic branch September 1, 2021 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

saturating arithmetic builtins: add, sub, mul, shl #9619

saturating arithmetic builtins: add, sub, mul, shl #9619

travisstaloch commented Aug 24, 2021

travisstaloch commented Aug 24, 2021

Snektron commented Aug 25, 2021

andrewrk commented Aug 25, 2021

travisstaloch left a comment

travisstaloch commented Aug 25, 2021

matu3ba left a comment

andrewrk left a comment

saturating arithmetic builtins: add, sub, mul, shl #9619

saturating arithmetic builtins: add, sub, mul, shl #9619

Conversation

travisstaloch commented Aug 24, 2021

travisstaloch commented Aug 24, 2021

Snektron commented Aug 25, 2021

andrewrk commented Aug 25, 2021

travisstaloch left a comment

Choose a reason for hiding this comment

travisstaloch commented Aug 25, 2021

matu3ba left a comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment