Closed charles-rs closed 11 months ago
sampsyo
commented
1 year ago Awesome!!! Thanks for getting this PR started!!
One medium-sized thing that would be helpful here would be to reuse the existing benchmark code (benchmarks/ in repo root) instead of having separate copies in here. I'll also look into getting this going on my machine in the mean time… if we get something roughly reproduced, this is probably worth merging.
charles-rs
commented
1 year ago Awesome!!! Thanks for getting this PR started!!
One medium-sized thing that would be helpful here would be to reuse the existing benchmark code (
benchmarks/in repo root) instead of having separate copies in here. I'll also look into getting this going on my machine in the mean time… if we get something roughly reproduced, this is probably worth merging.
i don't think there is any reason this should be difficult; we should be able to just use the config files and point them at the other benchmarks; i just need to rebuild all of the 6120 tools which i might not do tonight lol
charles-rs
commented
1 year ago ok it looks like it is pretty easy to use the existing benchmarks, so i got rid of the redundant one. this also exposed a "bug" where if you pass something like "23" to a program expecting a float it won't work since i was depending on seeing a decimal to parse floats
sampsyo
commented
1 year ago I've attempted to fix up a few things that weren't working on my end. In 76949d92e6c00ccb784c204f30bae6dc045210fe, I used built-in GNU Make functionality instead of shelling out to stuff, which fixed a few mysterious problems. In 06d5baf9fd910865367fca2d2767738a05048304, I bypassed the shell script srcgen.sh, which was just generating filenames and dispatching to awk, and instead had the Makefile supply those filenames directly to the awk script. @charles-rs, any chance you could check that things still work on your end?
One thing I'm still confused about: it looks like this PR has checked-in generated header files. In fact, it has them twice: once under lib and once under src. I think that they should probably be checked in zero times… and just generated during the build. Was that your intent, @charles-rs? If so, which directory are they supposed to go in? We can delete & ignore the generated files, but I want to make sure I'm putting them in the right place. (And TBH I don't understand why there is a lib directory at all, separate from src, so I thought I'd check.)
It also looks like there are a few other generated files that should probably be removed, in eval, but wanted to check before removing those also.
charles-rs
commented
1 year ago Thanks so much for getting this working on another system!!
I had to make one more change to the awk shebang invocation to get it to work in my environment, so if you could double check that that would be great
the generated headers were checked in because they weren't originally generated -- good catch
the reason for a separate src and lib directory is that lib contains the (quite minimal) runtime that you need to link against if you are using the compiler feature. The headers aren't even needed here though, so my bad.
re: eval/
this is really a directory with stuff in it that i used for my report. I think it's better to just be rid of it if that's ok with you
sampsyo
commented
1 year ago It took me a good long while, but I finally got another chance to take a look at this! Thanks again for helping move it forward. 🙏
I made a couple of minor changes:
turnt -j).emission.{h,cbak}, which don't seem to be used. (Everything still seems to compile and run, anyway.)I wanted to see how we're doing on correctness, so I ran all the benchmarks with turnt -j -e fastbrili benchmarks/**/*.bril. There are a few failures, but actually not that many:
1..70 ok 1 - benchmarks/core/ackermann.bril fastbrili ok 2 - benchmarks/core/armstrong.bril fastbrili ok 3 - benchmarks/core/binary-fmt.bril fastbrili ok 4 - benchmarks/core/birthday.bril fastbrili ok 5 - benchmarks/core/bitshift.bril fastbrili ok 6 - benchmarks/core/bitwise-ops.bril fastbrili ok 7 - benchmarks/core/catalan.bril fastbrili ok 8 - benchmarks/core/check-primes.bril fastbrili ok 9 - benchmarks/core/collatz.bril fastbrili ok 10 - benchmarks/core/digital-root.bril fastbrili ok 11 - benchmarks/core/dot-product.bril fastbrili ok 12 - benchmarks/core/euclid.bril fastbrili ok 13 - benchmarks/core/fact.bril fastbrili ok 14 - benchmarks/core/factors.bril fastbrili ok 15 - benchmarks/core/fitsinside.bril fastbrili ok 16 - benchmarks/core/fizz-buzz.bril fastbrili ok 17 - benchmarks/core/gcd.bril fastbrili ok 18 - benchmarks/core/hanoi.bril fastbrili ok 19 - benchmarks/core/is-decreasing.bril fastbrili ok 20 - benchmarks/core/lcm.bril fastbrili ok 21 - benchmarks/core/loopfact.bril fastbrili ok 22 - benchmarks/core/mod_inv.bril fastbrili ok 23 - benchmarks/core/orders.bril fastbrili ok 24 - benchmarks/core/palindrome.bril fastbrili ok 25 - benchmarks/core/pascals-row.bril fastbrili ok 26 - benchmarks/core/perfect.bril fastbrili ok 27 - benchmarks/core/primes-between.bril fastbrili ok 28 - benchmarks/core/pythagorean_triple.bril fastbrili ok 29 - benchmarks/core/quadratic.bril fastbrili ok 30 - benchmarks/core/recfact.bril fastbrili ok 31 - benchmarks/core/rectangles-area-difference.bril fastbrili ok 32 - benchmarks/core/relative-primes.bril fastbrili ok 33 - benchmarks/core/reverse.bril fastbrili ok 34 - benchmarks/core/sum-bits.bril fastbrili ok 35 - benchmarks/core/sum-check.bril fastbrili ok 36 - benchmarks/core/sum-divisors.bril fastbrili ok 37 - benchmarks/core/sum-sq-diff.bril fastbrili ok 38 - benchmarks/core/totient.bril fastbrili ok 39 - benchmarks/core/up-arrow.bril fastbrili not ok 40 - benchmarks/float/conjugate-gradient.bril fastbrili # differing: benchmarks/float/conjugate-gradient.out ok 41 - benchmarks/float/cordic.bril fastbrili not ok 42 - benchmarks/float/euler.bril fastbrili # differing: benchmarks/float/euler.out ok 43 - benchmarks/float/mandelbrot.bril fastbrili not ok 44 - benchmarks/float/n_root.bril fastbrili # differing: benchmarks/float/n_root.out not ok 45 - benchmarks/float/newton.bril fastbrili # differing: benchmarks/float/newton.out not ok 46 - benchmarks/float/norm.bril fastbrili # differing: benchmarks/float/norm.out not ok 47 - benchmarks/float/pow.bril fastbrili # differing: benchmarks/float/pow.out ok 48 - benchmarks/float/ray-sphere-intersection.bril fastbrili not ok 49 - benchmarks/float/riemann.bril fastbrili # differing: benchmarks/float/riemann.out not ok 50 - benchmarks/float/sqrt.bril fastbrili # differing: benchmarks/float/sqrt.out not ok 51 - benchmarks/long/function_call.bril fastbrili # missing: benchmarks/long/function_call.out ok 52 - benchmarks/mem/adj2csr.bril fastbrili ok 53 - benchmarks/mem/adler32.bril fastbrili ok 54 - benchmarks/mem/binary-search.bril fastbrili ok 55 - benchmarks/mem/bubblesort.bril fastbrili ok 56 - benchmarks/mem/csrmv.bril fastbrili ok 57 - benchmarks/mem/eight-queens.bril fastbrili ok 58 - benchmarks/mem/fib.bril fastbrili ok 59 - benchmarks/mem/major-elm.bril fastbrili ok 60 - benchmarks/mem/mat-mul.bril fastbrili ok 61 - benchmarks/mem/max-subarray.bril fastbrili ok 62 - benchmarks/mem/primitive-root.bril fastbrili ok 63 - benchmarks/mem/quickselect.bril fastbrili ok 64 - benchmarks/mem/quicksort-hoare.bril fastbrili ok 65 - benchmarks/mem/quicksort.bril fastbrili ok 66 - benchmarks/mem/sieve.bril fastbrili ok 67 - benchmarks/mem/two-sum.bril fastbrili ok 68 - benchmarks/mem/vsmul.bril fastbrili not ok 69 - benchmarks/mixed/cholesky.bril fastbrili # differing: benchmarks/mixed/cholesky.out not ok 70 - benchmarks/mixed/mat-inv.bril fastbrili # differing: benchmarks/mixed/mat-inv.out
The only failure mode I noticed while spot-checking is about float printing. For example:
$ turnt -e fastbrili --diff benchmarks/float/conjugate-gradient.bril
1..1
--- benchmarks/float/conjugate-gradient.out 2023-09-08 13:36:07
+++ /var/folders/dj/jj7bq9q570908ggkt0vcfvch0000gn/T/tmprv_jbljs 2023-12-03 16:07:20
@@ -1,3 +1,3 @@
-5.00000000000000000
-3.00000000000000000
-2.33333333333333348
+5
+3
+2.3333333333333335
not ok 1 - benchmarks/float/conjugate-gradient.bril fastbrili # differing: benchmarks/float/conjugate-gradient.outUnfortunately, this line doesn't quite cut it: https://github.com/sampsyo/bril/blob/96aca054ccf5ee774789bfb1903ff0aba7cb8b16/fastbril/lib/lib.c#L18
But we may be able to fix it by borrowing the silliness I worked out for Brilift: https://github.com/sampsyo/bril/blob/daaff284fdaee0319ab8cdcdaa9c620606125889/brilift/rt.c#L20-L32
In fact, someday maybe we should share the same runtime source code between the two, since essentially the identical functionality is required in both implementations.
Anyway, I think I would like to merge this now, if @charles-rs approves of the above changes! We can address the two things I have in mind in separate PRs:
…and then maybe we can enable testing in CI.
charles-rs
commented
11 months ago thanks so much for doing this! sorry it fell off the radar for a bit. Good call on the emission files, those were for a separate (incomplete) feature. This looks good to me to merge!
sampsyo
commented
11 months ago Awesome!! Thanks again for the really cool interpreter!!!!
like
brili, but faster. hopefully reasonably correct