-
Notifications
You must be signed in to change notification settings - Fork 13
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
Let's Write a Qi Compiler! #74
Conversation
06c0e24
to
d8616bc
Compare
Current performance comparison of individual forms with a priori (main branch) performance (repeated the same analysis that was done a little while back):
Summary: The criteria for displaying a relative performance factor other than 1 was r < 0.75x or r > 1.5x. (This was done on the on the latest |
To be fair, the one's that got slower mostly got slow by large factors (>=4, >=40)! |
Yeah it's just to give us an idea of progress rather than a report card. I'd love to get them all to <= 1 prior to merging this! |
Alrighty, we're getting the ball rolling on this again. I'll go ahead and merge #95 and then we can return to #76 and aim to Beat Racket™️ on one benchmark. That would bring us to the close of the compiler project where we can do the final review and merge this work, and that would then set us up for further optimizations over time on the main branch 🙂 |
Hello everyone! You may have noticed that the We are targeting the solstice, i.e. Dec 22, as the release date for Qi 4 -- a little under two weeks -- and it coincides with a Qi meeting as it's on a Friday. If you have time to review this PR, that would be much appreciated. Since it's such a huge PR, here's a quick overview of things as a refresher -- you could probably also refer to the meeting notes over the last year for additional context and the general story of how we got here. Overview of new operation: At a high level, when we write a flow, it first expands that expression to the Qi core language (e.g. Where is all this happening in the code, you might ask? Well, I've updated the source code layout wiki to include a comprehensive overview of every module in the source tree, for your reference. You could try things locally if you like (after checking out this branch):
If you have any Qi codebases (esp. @benknoble and @NoahStoryM ), now is a great time to test those against this compiler branch. There are some known backwards incompatibilities, and if you happen to run into any unknown ones that would be good to know 😆 Here are some specific items that may need review:
There are still a few items we'd like to do before release:
That's all I can think of for the moment. I'll aim to keep the PR description up to date with outstanding items. Thanks! Let's get this compiler out into the wild! 🐺 🐅 🌲 |
The two major things I'm going to go look for are
Of course, I've got to get my install switched over to this branch for that to happen and, well, last time I recall that took me some effort. But this time I'll try out the command that converts a package install to a Git clone, if I can point it to my existing clone. P.S. It would probably be good to take care of the merge conflicts sooner rather than later. |
@benknoble If you can also look at normalization that'd be great! Last time you found a bunch of issues with it, soooo, I think it would give us more confidence if you signed off on it 😄 I'm also curious how the switch-over goes with the install. IME it's been a matter of uninstalling any existing version of Qi (likely with And re: merge conflicts, agreed, I'm planning to rebase this branch soon (I will give a heads up). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some normalization comments.
qi-lib/flow/core/normalize.rkt
Outdated
[(thread _0 ... (amp f) (amp g) _1 ...) | ||
#'(thread _0 ... (amp (thread f g)) _1 ...)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not generally sound (I'm not sure if this is the smallest example):
> (~> (3) (>< (-< add1 sub1)) (>< (-< sub1 add1)))
3
5
1
3
> (~> (3) (>< (~> (-< add1 sub1) (-< sub1 add1))))
sub1: arity mismatch;
the expected number of arguments does not match the given number
expected: 1
given: 2
[,bt for context]
qi-lib/flow/core/normalize.rkt
Outdated
[(relay (~datum _) ...) | ||
#'_] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more permissive than current Qi:
> (~> (3) (relay _ _ _))
relay: arity mismatch;
the expected number of arguments does not match the given number
expected: 0
given: 2
[,bt for context]
qi-lib/flow/core/normalize.rkt
Outdated
[(thread _0 ... (~datum _) _1 ...) | ||
#'(thread _0 ... _1 ...)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is covered by a previous case on line 46.
qi-lib/flow/core/deforest.rkt
Outdated
(define (prettify-flow-syntax stx) | ||
(syntax-parse stx | ||
#:datum-literals (#%host-expression esc #%blanket-template #%fine-template) | ||
(((~literal thread) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know why this thread
has to be literal; in the rest of the compiler it's matched by datum AFAICT. Though perhaps
qi/qi-lib/flow/core/compiler.rkt
Lines 85 to 89 in 98c82c1
(define (rewrite-all-bindings stx) | |
(find-and-map/qi (syntax-parser | |
[((~datum as) x ...) | |
#:with (x-val ...) (generate-temporaries (attribute x)) | |
#'(thread (esc (λ (x-val ...) (set! x x-val) ...)) ground)] |
thread
with a binding (from racket
)?
I remembered the trick by checking the docs. I went to Qi clone and checked out the main branch (really the commit hash needs to match, or it needs to be possible to |
Address code review comments
Hi folks, if you're still planning to review this, please get your reviews in by tomorrow. If there are no further blockers, we will aim to merge this on Friday morning PST 😁 . At this stage, if you have comments that aren't blockers, they may be addressed after release, but they are still appreciated. Thanks! |
We don't anticipate further code changes before tomorrow's release, so now is a great time to test against this branch with Frosthaven (@benknoble ), Qi-Cat (@NoahStoryM ), Relation (@countvajhula ) and Qi-Circuit (@chansey97 ). For instructions on how to test against a specific repo branch, this comment (and the quoted text) may be helpful. Thanks! 😄 |
I had to pass
I can confirm that, after also |
Ah, please ignore the dependency warnings. I misunderstood the output of |
Update docs for Qi 4
Summary of Changes
Integration branch for the compiler work. This will eventually be merged into main when the compiler is ready.
See Qi Compiler Meeting Notes.
Work Remaining
for-label
kw-helper
is being correctly used in left- and right-currying cases (see#%blanket-template
code generation incompiler.rkt
) [Ben/Noah?]WIP
Already Done
(reverse-chronological, most recently completed at the top)
relation
library [Sid]esc
(and implicitly escaped like just usingdisplayln
),effect
andgen
should be honored (do these interact with deforestation or normalization?) [Ben/Dominik?]- [ ] Add docs for syntax-specPre-Merge Checklist
all-defined-out
-- see all-defined-out does not work with for-space racket/racket#4154)relation
library)Public Domain Dedication