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 firstEffectfulTerm #5361

Merged
merged 1 commit into from
May 26, 2023
Merged

Fix firstEffectfulTerm #5361

merged 1 commit into from
May 26, 2023

Conversation

zliu41
Copy link
Member

@zliu41 zliu41 commented May 25, 2023

In the Apply case (and similarly the Constr case), firstEffectfulTerm only looks at the function, not the argument, which is both inefficient and incorrect.

An example where it is inefficient:

let !a = error "a" in (\x -> x) a

The inliner is unable to inline a, because firstEffectfulTerm ((\x -> x) a) returns Nothing, but it should.

An example where it is incorrect:

let !a = error "a" in
let !b = (\x -> x) (error "b")
 in a b

The inliner incorrectly inlines a, changing the semantics of the program, because firstEffectfulTerm (let !b = (\x -> x) (error "b") in a b) returns Just a, which is, in turn, because firstEffectfulTerm ((\x -> x) (error "b")) returns Nothing.

This PR fixes it. There are some cost increases because previously it was inlining things that shouldn't have been inlined. That said, we should have a non-conservative mode that allows a to be inlined in the second example.

@zliu41 zliu41 requested review from michaelpj and a team May 25, 2023 21:58
@michaelpj
Copy link
Contributor

because firstEffectfulTerm (let !b = (\x -> x) (error "b") in a b) returns Just a, which is, in turn, because firstEffectfulTerm ((\x -> x) (error "b")) returns Nothing.

Bad past Michael! Conflating "we know there is no effectful term" with "we don't know whether there is an effectful term"!

{- |
Try to identify the first sub term which will be evaluated in the given term and
which could have an effect. 'Nothing' indicates that we don't know, this function
is conservative.
which could have an effect. 'Nothing' indicates that there's no term to evaluate.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have been inclined to fold this in as an alternative into FirstEffectfulTerm, but this is also okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this gives you the alternative instance that you want...

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly.

@zliu41 zliu41 merged commit e2f664e into master May 26, 2023
3 checks passed
@zliu41 zliu41 deleted the zliu41/firstEffectfulTerm branch May 26, 2023 15:55
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.

None yet

2 participants