Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bignums and flonums #63

Closed
wants to merge 43 commits into from
Closed

Bignums and flonums #63

wants to merge 43 commits into from

Conversation

l4haie
Copy link
Collaborator

@l4haie l4haie commented May 15, 2024

No description provided.

@l4haie l4haie marked this pull request as draft May 15, 2024 14:17
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Re-regarde surtout les appels à equal?. En testant de mon coté, ça crée des boucles infini. Pour tester, tu peux rajouter des display avant chaque test pour savoir lequel boucle à l'infini

src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Really good ! I'll review the rest later

src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Secound round of review ! Really nice work:)

src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Good job !

src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
src/bn.scm Outdated Show resolved Hide resolved
@leo-ard leo-ard marked this pull request as ready for review June 3, 2024 16:28
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

These are some quick things I've seen !

src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Un petit review, je regarderais le reste quand j'aurais plus de temps !

bn0)
((##eqv? (##bn< bn0 a) (##bn< bn0 b)) ;; positive quotient? (same parity)

;; if b is a fixnum, we need to pad both a and b with a 0 for the
Copy link
Collaborator

Choose a reason for hiding this comment

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

La condition est un peu bizarre. Tu devrait implémenté l'algorithme de division entre bignum(dividande) et un fixnum (diviseur) et l'utilisé dans bn-quotient. C'est un cas très fréquent de divisé par un fixnum, on veut que ce soit rapide.

(##quotient a b) ;; no need to normalize
(bn-norm (##bn-quotient (fixnum->bignum a) (fixnum->bignum b)))))

(define (##bn-quotient a b)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Techniquement l'algorithme retourne le résultat de la division et le reste de la division. Peut-etre changer le nom par ##bn-quotient-reminder ?

src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved

(let* ((d (##dummy-bn-quotient (##bn- bn-base bn1) (bn-get b 0 (##- 0 1)))) ;; normalize a and b
(_a (##bn* a d))
(_b (##bn* b d))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quand tu choisiras d comme une puissance de 2, alors les multiplications deviendrons des bitshift (plus rapide)

Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Très bien ! Quelques commentaires, surtout concernant quels valeurs sont des bignums et lesquels sont des fixnums

src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
Comment on lines 536 to 561
(let loop ((j m) ;; probably never will be a bignum
(q bn0)
(a _a))
(if (##< j 0)
q
(let* ((top-a (bn-get a j (##+ b-bits 1))) ;; (a_j+n ... a_j) i.e. |n|+1 digits
(lower-a (bn-get a 0 j)) ;; (a_j-1 ... a_0)
(top3-a ;; top 3 bits from (a_j+n ... a_j), some could be 0s
(bn-get top-a (##- (bn-length top-a) 3) 3)))

(if (##bn< (bn-next top3-a) b_n-1) ;; quotient is 0?
(loop (##- j 1) (bn-cons 0 q) a)
(let* ((q-hat-estimate (calculate-q-hat top3-a b_n-1 b_n-2))
(q-hat (add-back top-a _b q-hat-estimate))
(new-top-a (##bn- top-a (##bn* (bn-cons q-hat bn0) _b))))

;; FIXME, steps missing in the add-back part of the algorithm
;; (dif (bn- q-hat q-hat-estimate)) ;; number of add back iterations
;; (_top-a (if (bn= bn0 dif) ;;
;; __top-a
;; (bn-concatenate __top-a dif)))
;; (bn-concatenate (no-carry-bn+ (bn* dif _b) __top-a) dif)))

(loop (##- j 1)
(bn-cons q-hat q)
(bn-concatenate lower-a new-top-a)))))))))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Au lieux d'avoir un conteur j et d'indexer j dans le bignum avec bn-get à chaque tour de boucle, tu peux faire une optimization.

Si tu vois le bignum comme une liste, tu peux inversé la liste et bouclée dessus en prenant les deux première valeurs. Ça te sauve beaucoup d'indexation de la liste avec bn-get

src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
src/lib/bignum.scm Outdated Show resolved Hide resolved
Copy link
Collaborator

@leo-ard leo-ard left a comment

Choose a reason for hiding this comment

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

Quelques petits commentaire rapide :)

(q-hat (##quotient div b_n-1))
;; (r-hat (##remainder div b_n-1)))
(r-hat (##modulo div b_n-1)))
(r-hat (##modulo div b_n-1)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be interesting to see the difference in performance when ##modulo is a primitive instead of a function. I think the improvement would be significative

Comment on lines 526 to 536
(##< (##+ (##* base r-hat) a_j+n-2)
(##* q-hat b_n-2)))
(let ((q-hat (##- q-hat 1))
(r-hat (##+ r-hat b_n-1)))
(if (and (##< r-hat base)
(or (##< base-1 q-hat)
(##< (##+ (##* base r-hat) a_j+n-2)
(##* q-hat b_n-2))))
(##- q-hat 1)
q-hat))
q-hat)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have the impression that this can be optimized further to avoid repetition, but I'm not quite sure how to do it

@@ -445,11 +447,11 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Commentaire pour bn-concatenate, c'est une fonction équivalente à append pour les listes. Tu peux faire en sorte que la fonction est "tail call" en rajoutant un argument. On avait vu ça dans le cours de compilation ;)

@l4haie l4haie changed the title draft PR for bignums Bignums Jul 3, 2024
@l4haie l4haie changed the title Bignums Bignums and flonums Jul 18, 2024
@leo-ard leo-ard deleted the branch udem-dlteam:dev September 10, 2024 20:46
@leo-ard leo-ard closed this Sep 10, 2024
@leo-ard
Copy link
Collaborator

leo-ard commented Sep 10, 2024

@l4haie Apparently, deleting the dev branch closed this PR. You will need to create a new pull request targeting main

@l4haie l4haie deleted the bignums branch September 22, 2024 14:17
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.

2 participants