Racket list deforestation #175
Conversation
| [(_ [op (f ...) g ...] rest ...) (op f ... (inline-compose1 rest ...) g ...)] | ||
| )) | ||
|
|
||
| (define-syntax inline-consing |
There was a problem hiding this comment.
An explanatory comment would be great (similar to above)
| range | ||
|
|
||
| filter | ||
| map |
There was a problem hiding this comment.
Now that we are providing Qi bindings, for higher order functions I believe the f position(s) should be floe rather than expr. That is:
Not:
(~> (filter (λ (v) (= 0 (remainder v 3)))) (map (λ (v) (+ v 2))))
But instead:
(~> (filter (~> (remainder 3) (= 0))) (map (+ 2)))
Simple uses of function identifiers would be the same as for uses of Racket's map and filter
(~> (filter odd?) (map sqr))
But if a (Racket) lambda is desired it would need to be explicitly escaped:
(~> (map (esc (λ (v) (+ v 2)))))
| "passes.rkt") | ||
| "../strategy.rkt" | ||
| "../private/form-property.rkt") | ||
| "../passes.rkt") |
There was a problem hiding this comment.
Moving the passes under compiler/ 👌
| @@ -0,0 +1,298 @@ | |||
| #lang racket/base | |||
There was a problem hiding this comment.
Nice job separating out the matching/validation from the production. It should help us when we get to the point of moving the matching into the expander.
| ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; | ||
| ;; Producers | ||
|
|
||
| (define-syntax-class fsp-range |
There was a problem hiding this comment.
If we are going to use abbreviated names, the abbreviations should be clarified in comments, e.g. something like "In these syntax classes, fsp stands for fusable stream producer. These classes match functions that produce a sequence of values, such as range, and they annotate the syntax with attributes that will be used in the compiler to apply optimizations."
|
|
||
| ;; Performs deforestation rewrite on the whole syntax tree. | ||
| (define-and-register-deforest-pass (deforest-pass ops ctx) | ||
| (syntax-parse (reverse ops) |
There was a problem hiding this comment.
Would prefer using define-and-register-pass directly here rather than use a macro we don't anticipate another need for. To keep it modular, this (syntax-parse ...) bit could be moved into a locally-defined function.
There was a problem hiding this comment.
See above - this one will be also used in the named-let implementation in the (near) future.
| 'take ctx | ||
| #f | ||
| src | ||
| ) '())) |
There was a problem hiding this comment.
nit: parentheses are weird here. Would recommend keeping to Racket's conventions of not having dangling parens here.
There was a problem hiding this comment.
Will do in the integration branch as a new PR streamlining the comments across the compiler.
|
The more I think about it, I do not like providing bindings from Qi that have Racket syntax (which we discussed in today's meeting, in a separate My preference would be to work out the proper interface together as part of merging this PR, and in addition, provide only Qi-syntaxed bindings from Qi libraries like In any case, having a clean interface now would unblock the deforestation efforts in earnest and I wouldn't be paranoid and watching over your shoulder @dzoep . Until then, I hesitate 😅 |
There was a problem hiding this comment.
Apologies in advance for this absolutely massive review; I did this offline over a week ago by annotating the results of git switch racket-list-deforestation && git format-patch --cover-letter main -o patches, so it built up. I did walk commit by commit, so you might want to do the same to get context on my review comments.
-
I get a compilation error in qi-sdk when building this branch with
raco setup -l qi:raco setup: error: during making for <pkgs>/qi-sdk/benchmarks/nonlocal/qi raco setup: qi-sdk/benchmarks/nonlocal/qi/main.rkt:23:9: module: identifier already required raco setup: at: range raco setup: in: qi/list raco setup: compiling: <pkgs>/qi-sdk/benchmarks/nonlocal/qi/main.rkt -
FHM tests pass (haven't tried actually running it yet)
-
Second Sid's comments about standard Racket parentheses: all closers belong on
the same line. I would also prefer to see idiomatic square brackets (raco fmtcan help with this, but I don't always agree with the way it shoves
things onto one line when I want the separation). This occurs in more places
than I think I saw fixed. -
On a related note, I'm not a fan of long
(begin-…blocks: it can be hard to
eyeball things when the(beginis way off the screen. I've suggested one or
two ways to try to slim them down, but I don't know if you already fixed one
of them. -
Commit hygiene: I'd prefer each commit to make sense on its own (and ideally
build + pass tests), so I've left targeted comments about what should be
squashed or fixed up where, etc. It's probably going to be challenging at this
stage; you might want to create a v2 branch and slowly cherry-pick commits,
fix them up, etc., instead of trying a complicated interactive rebase. (It's
the same effect and will have all the same conflicts; it might be easier to
keep track of what's going on that way.) An extreme alternative option would be to squash-merge this PR, at which point I would beseech you to write a solid message describing the whole PR as if it were a single commit so that Sid (or whoever merges) can paste that in (GitHub's "default squash message" is worse that useless).
Deforest all variants of cad*r: - car - cadr - caddr - cadddr - caddddr - cadddddr Deforest (using the same underlying implementation) list-ref as well.
- split syntax matching from syntax production - improve naming of syntax classes - remove unused template variables
dzoep
force-pushed
the
racket-list-deforestation
branch
from
ce0bc49
to
121b212
Compare
|
I won't have time ahead of today's meeting, but I do intend to resolve any of my comments that are no longer needed. (If anyone else happens to go through them, that's fine too.) |
- preliminary splitting of the compiler into separate modules for separate passes - update tests to reflect new paths
- rename compiler "passes" subdirectory to "compiler" - strip the passes modules file name pass- prefix
Towards addressing feedback on discord from @dented42
…s fixes and tests for them.
…ntax and do not provide them.
- scribblings for qi/list module - scribble the new literals for matching in deforestation pass - ensure for-label bindings in the generated documentation - new bindings.rkt module
…ler meeting on 2024-06-21. - add detailed explanation for inline-consing syntax - use Racket's conventions for parentheses - add description of fsp-, fst-, and fsc- prefixes - move define-and-register-deforest-pass and related to separate module, add comments
…on (CPS) implementation.
dzoep
force-pushed
the
racket-list-deforestation
branch
from
121b212
to
016ab41
Compare
|
I've marked many comments resolved, but left open the ones that aren't addressed and that I think should still be (or at least should be considered). I also still get an error in qi-sdk when trying to build locally. |
|
Also, it looks like there are some whitespace oddities. For example, try using the Here's the resulting fix list: |
|
For the conflicts, I started the merge and then ran In fact, it's probably a good idea to try rebasing on top of the latest |
Summary of Changes
Public Domain Dedication
(Why: The freely released, copyright-free work in this repository represents an investment in a better way of doing things called attribution-based economics. Attribution-based economics is based on the simple idea that we gain more by giving more, not by holding on to things that, truly, we could only create because we, in our turn, received from others. As it turns out, an economic system based on attribution -- where those who give more are more empowered -- is significantly more efficient than capitalism while also being stable and fair (unlike capitalism, on both counts), giving it transformative power to elevate the human condition and address the problems that face us today along with a host of others that have been intractable since the beginning. You can help make this a reality by releasing your work in the same way -- freely into the public domain in the simple hope of providing value. Learn more about attribution-based economics at drym.org, tell your friends, do your part.)