Add mergable changes from platforms #789
Conversation
| :precision ,(table-row-precision row) | ||
| :herbie-conversions ,(table-row-conversions row) | ||
| ,@(if (eq? (table-row-pre row) 'TRUE) '() `(:pre ,(table-row-pre row))) | ||
| ,@(if (equal? (table-row-preprocess row) empty) '() `(:herbie-preprocess ,(table-row-preprocess row))) | ||
| ,@(if (table-row-target-prog row) `(:alt ,(table-row-target-prog row)) '()) | ||
| ,(table-row-output row))) | ||
| ,@(if (table-row-target-prog row) `(:herbie-target ,(table-row-target-prog row)) '()) |
There was a problem hiding this comment.
I don’t think this is right, we just switched to :alt
|
Any idea why test are failing? Also merge main so nightlies can pass, before we merge. |
| (define-accelerator (expm1 real) real (lambda (x) (- (exp x) 1))) | ||
| (define-accelerator (log1p real) real (lambda (x) (log (+ 1 x)))) | ||
| (define-accelerator (hypot real real) real (lambda (x y) (sqrt (+ (* x x) (* y y))))) | ||
| (define-accelerator (fma real real real) real (lambda (x y z) (+ (* x y) z))) | ||
| (define-accelerator (erfc real) real (lambda (x) (- 1 (erf x)))) |
There was a problem hiding this comment.
This lost the ability to provide interval versions, like ival-log1p. This is what's causing the test failures. There are a two possible fixes:
- The most expedient fix is to bring that capability back, it's not hard.
- The longer but eventually better fix is to desugar accelerators on the way into sampling, and then sugar them back inside sampling. This is the long-term plan in sampling.
Luckily we can do both at once, so I say @broughjt you do the first one and I'll work on the second.
|
I think this is good, now that it's passing tests? I'd like to merge it if possible. |
| (define spec1 (expand-accelerators (prog->spec p1))) | ||
| (define spec2 (expand-accelerators (prog->spec p2))) |
There was a problem hiding this comment.
Ok, this is fine but not ideal—if you write a benchmark with expm1 in it Herbie might fail to sample it.
I guess this isn't the end of the world and we can merge now and wait for some later time to fix this properly.
| (for ([pt points]) | ||
| (with-check-info (['point (map list fv pt)]) | ||
| (define v1 (apply prog1 pt)) | ||
| (define v2 (apply prog1 pt)) | ||
| (define v2 (apply prog2 pt)) |
|
Any reason we're not merging? |
|
I looked at the most recent nightly run and the "simple fma test" and "Given's rotation SVD example, simplified" benchmarks are different. Notably these are the two benchmarks that use accelerators in the spec. So something there is still going wrong. |
Changes:
accelerator.rktRefactor of the accelerator APIcore/egg-herbie.rktExclude an egg-herbiercore/matcher.rkt,core/rr.rktMoverule-apply,rewrite-once, andrewrite-expressionsto a newrr.rktfilepatch.rktMake compatible with newexpand-acceleratorsinterfaceplatforms.rktChange names toplatform-impl-costandplatform-impl-reprplugin.rktChange internals module exports to match accelerator changes