check if unsigned numbers underflow #1025

Jobhdez · 2023-10-18T22:41:01Z

@Izaakwltn Hopefully this is not an issue that I made another pr. I had git issues with the other one.

This solves #921.

This is the interaction I just had in this new pr:

(coalton-toplevel 
		(declare add-8bit (U8 -> U8 -> U8))
		(define (add-8bit x y)
		  (+ x y))
		(declare sub-8bit (U8 -> U8 -> U8))
		(define (sub-8bit x y)
		  (- x y))
		(declare mul-8bit (U8 -> U8 -> U8))
		(define (mul-8bit x y)
		  (* x y))
		(declare fint (Integer -> U8))
		(define (fint x)
		  (fromInt x)))
; No value
COALTON-USER> (coalton (add-8bit 2 3))
5
COALTON-USER> (coalton (add-8bit 3 -4))
; Evaluation aborted on #<SIMPLE-ERROR "Unsigned value underflowed 8 bits." {1007939573}>.
COALTON-USER> (coalton (sub-8bit 3 2))
1
COALTON-USER> (coalton (sub-8bit 3 5))
; Evaluation aborted on #<SIMPLE-ERROR "Unsigned value underflowed 8 bits." {100835E453}>.
COALTON-USER> (coalton (mul-8bit 3 4))
12
COALTON-USER> (coalton (mul-8bit 3 -4))
; Evaluation aborted on #<SIMPLE-ERROR "Unsigned value underflowed 8 bits." {1008C8C2B3}>.
COALTON-USER> (coalton (fint 4))
4
COALTON-USER> (coalton (fint -2))
; Evaluation aborted on #<SIMPLE-ERROR "Unsigned value underflowed 8 bits." {10017D9693}>.

Thanks

Izaakwltn · 2023-10-18T23:12:23Z

I tested it and this provides correct underflow errors.

I think it would make sense to use cl:error instead of cl:cerror, and remove the continue-format-string since there is no wrapping operation defined. Currently, if you select 0: [CONTINUE] Continue, wrapping around. in the debugger, you get another error because it's still expecting a number and isn't getting one:

The value
  COMMON-LISP:NIL
is not of type
  (UNSIGNED-BYTE 16)
   [Condition of type COMMON-LISP:TYPE-ERROR]

Otherwise, this seems solid.

Izaakwltn · 2023-10-18T23:34:34Z

Looks good to me! I have no gripes.

I think there should be a future issue to have unsigned numbers error on overflow too, but that can be a separate thing.

I'm going to approve it but I'll wait to see if anyone else wants to review it before merging.

stylewarning

The PR on the whole isn't structured correctly. We are defining Num for the same unsigned types multiple times, once for wrapping behavior, and then for underflow behavior. We can only define them once.

library/math/num.lisp

stylewarning · 2023-10-19T16:20:02Z

library/math/num.lisp

+    `(define-instance (Num ,type)
+       (define (+ a b)
+         (lisp ,type (a b)
+	   (cl:if (cl:and (cl:< b 0) (cl:< a (cl:- 0 b)))


(< x 0) ==> (minusp x)

(- 0 x) ==> (- x)

stylewarning · 2023-10-19T16:21:11Z

library/math/num.lisp

+
+       (define (- a b)
+         (lisp ,type (a b)
+	   (cl:if (cl:and (cl:>= b 0) (cl:< a (cl:+ 0 b)))


b is guaranteed >= 0 for unsigned numbers

stylewarning · 2023-10-19T16:21:32Z

library/math/num.lisp

+    `(define-instance (Num ,type)
+       (define (+ a b)
+         (lisp ,type (a b)
+	   (cl:if (cl:and (cl:< b 0) (cl:< a (cl:- 0 b)))


b can never be negative for unsigned numbers

stylewarning · 2023-10-19T16:22:24Z

library/math/num.lisp

+
+       (define (* a b)
+	 (lisp ,type (a b)
+	   (cl:if (cl:or (cl:and (cl:and (cl:> b 0) (cl:< a 0)) (cl:< a (cl:/ 0 b)))


unsigned multiplication can't underflow

what do you mean by this? are you saying the code that i wrote doesnt detect the underflow or that in general unsigned multiplication doesnt underflow?

wouldnt this be an underflow: 2 * -3?

-3 isn't an unsigned number

stylewarning · 2023-10-19T16:28:00Z

(Incidentally this means we have no tests that actually look for correct overflow / underflow behavior.)

Jobhdez · 2023-10-19T20:07:53Z

@stylewarning thank you for this.

I will try to address your review as soon as I can. Thanks.

For the wrapper, I just have to make it so when the unsigned values underflow, the user can continue, if they want, with the highest number possible of the given unsigned type right?

so if two addition of U8 integers underflow, the wrapper needs to allow the user continue from the error with the wrapped number which in this case would be 255? correct? ust trying to make sure I am thinking the right thing. thanks

library/math/num.lisp

Jobhdez · 2023-10-20T21:16:17Z

@Izaakwltn addressed your review

stylewarning

Thanks for your patience in review.

This PR needs to modify define-num-wrapping's definition of - and fromInt only. Adding another define macro is not needed (and is actually redundant), and the other "handler" functionality is not all that useful outside of the scope of this change, so it can be eliminated.

stylewarning · 2024-01-31T22:08:02Z

library/math/num.lisp

@@ -161,6 +161,47 @@
 ;;; Num instances for integers
 ;;;

+(cl:defmacro %define-unsigned-underflow-handler (name bits)


i think this can be deleted

stylewarning · 2024-01-31T22:08:17Z

library/math/num.lisp

+       (64 (cl:the (cl:unsigned-byte ,bits) 18446744073709551615)))))
+
+
+(%define-unsigned-underflow-handler %unsigned-8-bit-underflow-handler 8)


these can be deleted

stylewarning · 2024-01-31T22:09:31Z

library/math/num.lisp

-  (define-num-wrapping UFix #.+unsigned-fixnum-bits+))
+  (define-num-wrapping UFix #.+unsigned-fixnum-bits+)
+
+  (define-unsigned-num-underflow U8 %unsigned-8-bit-underflow-handler 8)


these are re-defining Num for types that already have it. Above, define-num-wrapping is already defining - and the like for each of the listed types.

Jobhdez · 2024-02-02T06:21:23Z

Thanks for your patience in review.

This PR needs to modify define-num-wrapping's definition of - and fromInt only. Adding another define macro is not needed (and is actually redundant), and the other "handler" functionality is not all that useful outside of the scope of this change, so it can be eliminated.

mybad for the late reply. But your comment means that I only pretty much have take what I wrote in my function and move to the define-num-wrapping function? thanks

stylewarning · 2024-02-02T15:11:21Z

Yes, essentially.

Jobhdez · 2024-02-02T21:43:50Z

Yes, essentially.

Ok I’ll get it done ASAP.

Izaakwltn · 2024-03-04T18:11:44Z

At least one test failing due to the ecase, specifically in test iter-min-max:

While evaluating the form starting at line 3, column 0
  of #P"/__w/coalton/coalton/run-tests.lisp":
Unhandled SB-KERNEL:CASE-FAILURE in thread #<SB-THREAD:THREAD "main thread" RUNNING
                                              {1001090073}>:
  62 fell through COMMON-LISP:ECASE expression. Wanted one of (8 16 32 64).

Your ecase can get matches for bits that aren't 8, 16, 32, or 64.

I would recommend loading the coalton test suite and either running all tests locally or just running iter-min-max to debug.

Edit: in terms of the failing iterator test, I think the culprit is behavior in iter:range-decreasing.

Izaakwltn · 2024-03-05T20:02:36Z

I think +unsigned-fixnum-bits+ is sbcl specific, so the tests are passing in SBCL but still not in the other implementations:

Allegro:
https://github.com/coalton-lang/coalton/actions/runs/8151586516/job/22279665406?pr=1025#step:5:507

CCL:
https://github.com/coalton-lang/coalton/actions/runs/8151586516/job/22279665807?pr=1025#step:6:434

You might be able to do an #+sbcl exception for that approach and find another way to handle the other implementations, but if you find a uniform solution for all three types that would be optimal.

Jobhdez force-pushed the signal-underflow-error branch from 87b405a to e962184 Compare October 18, 2023 23:25

Izaakwltn approved these changes Oct 18, 2023

View reviewed changes

stylewarning requested changes Oct 19, 2023

View reviewed changes

Jobhdez force-pushed the signal-underflow-error branch from e962184 to 42ef9e3 Compare October 20, 2023 06:13

Izaakwltn reviewed Oct 20, 2023

View reviewed changes

library/math/num.lisp Outdated Show resolved Hide resolved

Izaakwltn reviewed Oct 20, 2023

View reviewed changes

library/math/num.lisp Outdated Show resolved Hide resolved

Jobhdez force-pushed the signal-underflow-error branch from 42ef9e3 to 37854e6 Compare October 20, 2023 21:13

Jobhdez force-pushed the signal-underflow-error branch from 37854e6 to ac8b9bc Compare October 21, 2023 15:07

stylewarning requested changes Jan 31, 2024

View reviewed changes

Jobhdez force-pushed the signal-underflow-error branch from ac8b9bc to a4a252c Compare March 4, 2024 05:00

Jobhdez requested a review from stylewarning March 4, 2024 05:06

Jobhdez force-pushed the signal-underflow-error branch from 429e5fa to 7665659 Compare March 5, 2024 05:38

Jobhdez closed this Mar 7, 2024

Jobhdez reopened this Mar 8, 2024

add unsigned numbers wrap around

2ba6cd6

Jobhdez force-pushed the signal-underflow-error branch from 7665659 to 2ba6cd6 Compare March 8, 2024 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check if unsigned numbers underflow #1025

check if unsigned numbers underflow #1025

Jobhdez commented Oct 18, 2023 •

edited

Izaakwltn commented Oct 18, 2023

Izaakwltn commented Oct 18, 2023

stylewarning left a comment

stylewarning Oct 19, 2023

stylewarning Oct 19, 2023

stylewarning Oct 19, 2023

stylewarning Oct 19, 2023

Jobhdez Oct 19, 2023

Izaakwltn Oct 19, 2023

stylewarning commented Oct 19, 2023

Jobhdez commented Oct 19, 2023

Jobhdez commented Oct 20, 2023

stylewarning left a comment

stylewarning Jan 31, 2024

stylewarning Jan 31, 2024

stylewarning Jan 31, 2024

Jobhdez commented Feb 2, 2024

stylewarning commented Feb 2, 2024

Jobhdez commented Feb 2, 2024

Izaakwltn commented Mar 4, 2024 •

edited

Izaakwltn commented Mar 5, 2024

		(64 (cl:the (cl:unsigned-byte ,bits) 18446744073709551615)))))


		(%define-unsigned-underflow-handler %unsigned-8-bit-underflow-handler 8)

check if unsigned numbers underflow #1025

Are you sure you want to change the base?

check if unsigned numbers underflow #1025

Conversation

Jobhdez commented Oct 18, 2023 • edited

Izaakwltn commented Oct 18, 2023

Izaakwltn commented Oct 18, 2023

stylewarning left a 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

Choose a reason for hiding this comment

stylewarning commented Oct 19, 2023

Jobhdez commented Oct 19, 2023

Jobhdez commented Oct 20, 2023

stylewarning left a 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

Jobhdez commented Feb 2, 2024

stylewarning commented Feb 2, 2024

Jobhdez commented Feb 2, 2024

Izaakwltn commented Mar 4, 2024 • edited

Izaakwltn commented Mar 5, 2024

Jobhdez commented Oct 18, 2023 •

edited

Izaakwltn commented Mar 4, 2024 •

edited