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

Fix record-ref reduction in cp0 #421

Merged
merged 1 commit into from
Apr 19, 2019
Merged

Conversation

gus-massa
Copy link
Contributor

In expressions like

(record-ref ... (begin (newline) (record ...)))

the current reduction was dropping the possible side effect expressions
in this case the (newline).

A complete example is

(run-cp0 (lambda (cp0 x) (cp0 (cp0 (cp0 x))))]) ; three times

(let ()
  (define-record mybox (val))
  (mybox-val (begin (display 1) (make-mybox 2))))

==> 2 instead of 12


As a side note, running only two times the cp0 pass, it is not enough to reduce some expressions like (mybox-val (make-mybox 2))). It is necessary to run cp0 three times. I don't know after expansions and inlining if this kind of pattern is common. I tried compiling with three times cp0 instead of two and the compilation time changes very little. Perhaps it could be useful to change the default.

@dybvig dybvig self-assigned this Apr 18, 2019
@dybvig
Copy link
Member

dybvig commented Apr 18, 2019

Thanks for the bug report and fix, which looks correct.

Please pull the common non-result-exp call outside of the if on line 4711 of cp0.ss and rebase.

Were you able to come up with a mat for the second of the two cases that were missing the non-result-exp calls? I tried a few things but something else seems to be pulling the effect out so that it was residualized properly without non-result-exp.

There are always things an extra pass of cp0 will pick up. My guess is cases like this aren't particularly common, and it takes only one or two calls to cp0 to when the field is immutable.

In expressions like
  (record-ref ... (begin (newline) (record ...)))
the reduction was dropping the possible side effect expressions
in this case the (newline).

 cp0.ss
@gus-massa gus-massa force-pushed the 19-4-Fix-Record-Ref branch from 382b035 to 5c50c5d Compare April 19, 2019 12:45
@gus-massa
Copy link
Contributor Author

Done.

I also changed in the test two equal? to member?, because there are two possible correct results.

@dybvig
Copy link
Member

dybvig commented Apr 19, 2019

Looks good, thanks.

@dybvig dybvig merged commit 53d09d9 into cisco:master Apr 19, 2019
mflatt pushed a commit to racket/ChezScheme that referenced this pull request Mar 24, 2021
mflatt pushed a commit to mflatt/ChezScheme that referenced this pull request Oct 10, 2023
Fix record-ref reduction in cp0

Original commit: 53d09d9
mflatt pushed a commit to mflatt/ChezScheme that referenced this pull request Oct 10, 2023
Fix record-ref reduction in cp0

Original commit: 53d09d9
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.

2 participants