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

Let's Write a Qi Compiler! #74

Merged
merged 441 commits into from
Jan 12, 2024
Merged

Let's Write a Qi Compiler! #74

merged 441 commits into from
Jan 12, 2024

Conversation

countvajhula
Copy link
Collaborator

@countvajhula countvajhula commented Sep 3, 2022

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

  • Investigate Qi forms still being unlinked in the docs, though they are now exported bindings (but in the Qi binding space) and required for-label
  • Validate that kw-helper is being correctly used in left- and right-currying cases (see #%blanket-template code generation in compiler.rkt) [Ben/Noah?]

WIP

Already Done

(reverse-chronological, most recently completed at the top)

Pre-Merge Checklist

Public Domain Dedication

  • In contributing, we relinquish any copyright claims on our contributions and freely release them into the public domain in the simple hope that they will provide value. (Why)

@countvajhula countvajhula changed the title Lets write a qi compiler Lets Write a Qi Compiler Sep 3, 2022
@countvajhula countvajhula changed the title Lets Write a Qi Compiler Lets Write a Qi Compiler! Sep 3, 2022
@countvajhula
Copy link
Collaborator Author

FYI @michaelballantyne @benknoble

@countvajhula countvajhula temporarily deployed to test-env September 15, 2022 04:02 Inactive
@countvajhula countvajhula force-pushed the lets-write-a-qi-compiler branch from 06c0e24 to d8616bc Compare September 24, 2022 20:31
@countvajhula countvajhula temporarily deployed to test-env September 24, 2022 20:36 Inactive
@countvajhula countvajhula temporarily deployed to test-env September 24, 2022 20:37 Inactive
@countvajhula
Copy link
Collaborator Author

Current performance comparison of individual forms with a priori (main branch) performance (repeated the same analysis that was done a little while back):

'(("live?" "55.71")
  ("count" "33.4")
  ("rectify" "29.76")
  ("none?" "14.48")
  ("all?" "12.97")
  ("AND" "11.77")
  ("any?" "10.79")
  ("OR" "9.69")
  ("any" "9.14")
  ("loop" "8.96")
  ("all" "7.56")
  ("partition" "7.5")
  ("none" "6.33")
  ("pass" "4.72")
  ("sieve" "4.51")
  ("inverter" "4.21")
  ("amp" "4.1")
  ("NAND" "4.09")
  ("or%" "1.86")
  ("select" "1.6")
  ("or" "1.59")
  ("and%" "1.57")
  ("not" "1.5")
  ("NOR" 1)
  ("NOT" 1)
  ("XNOR" 1)
  ("XOR" 1)
  ("and" 1)
  ("apply" 1)
  ("block" 1)
  ("bundle" 1)
  ("catchall-template" 1)
  ("clos" 1)
  ("collect" 1)
  ("crossover" 1)
  ("currying" 1)
  ("effect" 1)
  ("fanout" 1)
  ("feedback" 1)
  (">>" 1)
  ("<<" 1)
  ("gate" 1)
  ("gen" 1)
  ("group" 1)
  ("if" 1)
  ("input aliases" 1)
  ("loop2" 1)
  ("one-of?" 1)
  ("relay" 1)
  ("relay*" 1)
  ("sep" 1)
  ("switch" 1)
  ("tee" 1)
  ("template" 1)
  ("thread" 1)
  ("try" 1)
  ("unless" 1)
  ("when" 1)
  ("esc" "0.68")
  ("ground" "0.58")
  ("thread-right" "0.56"))

Summary:
23 slower
3 faster
35 same

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 add-bindings-to-qi branch)

@benknoble
Copy link
Collaborator

Summary:

**23 slower

3 faster

35 same**

To be fair, the one's that got slower mostly got slow by large factors (>=4, >=40)!

@countvajhula
Copy link
Collaborator Author

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!

@countvajhula countvajhula temporarily deployed to test-env January 6, 2023 21:05 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env January 12, 2023 20:49 — with GitHub Actions Inactive
@countvajhula countvajhula temporarily deployed to test-env May 12, 2023 19:41 — with GitHub Actions Inactive
@countvajhula
Copy link
Collaborator Author

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 🙂

@countvajhula
Copy link
Collaborator Author

countvajhula commented Dec 11, 2023

Hello everyone! You may have noticed that the first-optimizations branch was merged a few days ago (the actual one, this time!). So now, after more than a year of work, we're finally close to releasing this! 🏁 🏎️ 💨 And what's more, we're beating Racket on some benchmarks, as we set out to do!

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. switch is rewritten to if as part of this process, analogously to how cond is rewritten to if as part of Racket expansion). Then, the core language expression is compiled to produce Racket code which is then executed by the underlying Racket interpreter. The compilation phase is itself done in several stages. First, the input expression is normalized to a standard form ("Qi Normal form"). Then, that normalized expression is optimized (currently the only optimization we perform is deforestation, for list-based functional pipelines). And finally, the optimized expression is translated into Racket ("code generation step", fulfilled by the qi0->racket macro).

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):

make build
make test-flow 
make test-compiler
make profile-competitive

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:

  • Are there any inequivalencies in the normalization pass? Is there anything else it should be doing? Is it doing anything more than it should be?
  • Any concerns with termination of compiler passes? Currently we run each pass until it reaches a fixed point. We've been implicitly assuming it will always get there, but we don't know that for sure.
  • Any thoughts on the benchmarks we're using (in qi-sdk/profile/nonlocal)? Any ideas for other benchmarks?
  • Do the existing optimizations look OK?
  • Error messages, code layout, tests, coverage, anything else you can think of
  • What do you think of the new syntax for bindings?

There are still a few items we'd like to do before release:

  • documentation! Both user and developer docs.
  • code coverage is currently showing as 72% but that doesn't seem accurate. We'd like to fix that and hopefully bring it up to 100% soon.
  • bindings are not properly scoped with respect to the switch conditional form. We are considering this a blocker for release and hope to look into it soon (@michaelballantyne , we might need your help on it!)
  • incorporating competitive benchmarks into the live dashboard

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! 🐺 🐅 🌲

@benknoble
Copy link
Collaborator

The two major things I'm going to go look for are

  1. Do my tests pass (Frosthaven Manager and old Advent of Code)?
  2. Does the literal thread matching from recent notes actually produce some weirdness? (I expect there might be a way to write a program that doesn't do the obvious thing, such as (flow (thread add1)).)

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.

@countvajhula
Copy link
Collaborator Author

@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 raco pkg remove --force qi), and then make install within the local cloned Qi repo. After that, you'd need to make build each time you switch branches.

And re: merge conflicts, agreed, I'm planning to rebase this branch soon (I will give a heads up).

Copy link
Collaborator

@benknoble benknoble left a comment

Choose a reason for hiding this comment

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

Some normalization comments.

Comment on lines 31 to 32
[(thread _0 ... (amp f) (amp g) _1 ...)
#'(thread _0 ... (amp (thread f g)) _1 ...)]
Copy link
Collaborator

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]

Comment on lines 52 to 53
[(relay (~datum _) ...)
#'_]
Copy link
Collaborator

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]

Comment on lines 75 to 76
[(thread _0 ... (~datum _) _1 ...)
#'(thread _0 ... _1 ...)]
Copy link
Collaborator

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.

(define (prettify-flow-syntax stx)
(syntax-parse stx
#:datum-literals (#%host-expression esc #%blanket-template #%fine-template)
(((~literal thread)
Copy link
Collaborator

@benknoble benknoble Dec 12, 2023

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

(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)]
unintentionally introduced a thread with a binding (from racket)?

@benknoble
Copy link
Collaborator

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 raco pkg remove --force qi), and then make install within the local cloned Qi repo. After that, you'd need to make build each time you switch branches.

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 pull --ff-only to the right commit). Then raco pkg update --clone . qi (and respond a to the prompt) switched my install to my clone and re-setup dependent packages.

@countvajhula
Copy link
Collaborator Author

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!

@countvajhula
Copy link
Collaborator Author

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! 😄

@benknoble
Copy link
Collaborator

I had to pass --clean to raco setup when building this branch recently using make build-all, which spat out the following deps warnings:

raco setup: unused dependencies detected
  for package: "qi-lib"
  on packages:
   "syntax-spec-v1"
   "fancy-app"
   "macro-debugger"
raco setup: unused dependencies detected
  for package: "qi-test"
  on packages:
   "adjutor"
   "math-lib"
   "syntax-spec-v1"
   "qi-lib"
   "rackunit-lib"
   "testing-util-lib"
raco setup: unused dependencies detected
  for package: "qi-sdk"
  on packages:
   "cover-coveralls"
   "adjutor"
   "relation-lib"
   "math-lib"
   "qi-lib"
   "csv-writing"
   "cover"
   "cli"
   "collections-lib"
   "require-latency"
raco setup: unused dependency detected
  for package: "qi-probe"
  on package:
   "qi-lib"
raco setup: unused dependencies detected
  for package: "qi-doc"
  on packages:
   "scribble-abbrevs"
   "qi-probe"
   "qi-lib"
   "scribble-html-lib"
   "racket-doc"
   "sandbox-lib"
   "scribble-lib"
   "scribble-math"
   "metapict"

I can confirm that, after also raco setup frosthaven-manager advent2021, the tests raco test -xc -j 8 frosthaven-manager advent2021 passed. If I find any bugs while running the FHM GUI, I'll let you know ASAP (we play weekly, so earliest you'll hear is next week).

@benknoble
Copy link
Collaborator

Ah, please ignore the dependency warnings. I misunderstood the output of --clean.

@countvajhula countvajhula merged commit e66a062 into main Jan 12, 2024
14 checks passed
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.

6 participants