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

[PIR] Don't generate 'fixBy' if you don't need to #5954

Conversation

effectfully
Copy link
Contributor

Currently we generate fixBy (a combinator used for compiling mutual recursion) whenever we have any recursive functions in the PIR program, but most of those only need the basic Z combinator and no fancy fixBy stuff, so generating the latter is pointless and can be very confusing if you want to look at the output of the compiler without optimizations (in particular, DCE) turned on. Not generating fixBy if you don't need to is trivial, so let's do that.

@effectfully effectfully added enhancement Plutus IR No Changelog Required Add this to skip the Changelog Check labels May 4, 2024
@effectfully effectfully self-assigned this May 4, 2024
@effectfully effectfully requested a review from Unisay May 4, 2024 04:12
Copy link
Contributor

@Unisay Unisay left a comment

Choose a reason for hiding this comment

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

Unfortunately I can't see a change in a golden file that demonstrates how fixBy was generated before the change and how its not generated after.

Do you mind adding a golden file demonstrating the desired effect?

…to effectfully/plutus-ir/do-not-generate-fixBy-if-you-do-not-need-to
@effectfully
Copy link
Contributor Author

Unfortunately I can't see a change in a golden file that demonstrates how fixBy was generated before the change and how its not generated after.

Do you mind adding a golden file demonstrating the desired effect?

Well, I can't do that in this PR, because this PR removes generation of fixBy, so any new golden file will not contain it. But anyway, the idea was that #5939 adds such a test, so we'll have it soon without me needing to add it separately. Let's wait for #5939 to get merged and then I'll update things here.

@effectfully
Copy link
Contributor Author

But anyway, the idea was that #5939 adds such a test, so we'll have it soon without me needing to add it separately. Let's wait for #5939 to get merged and then I'll update things here.

I should've said explicitly when creating the PR though. Sorry!

@@ -79,6 +79,51 @@ applyFun = runQuote $ do
. lamAbs () x (TyVar () a)
$ apply () (var () f) (var () x)

{- Note [Recursion combinators]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I confused branches and added the Note in this one rather than the strictify-fix one. Doesn't matter much, I guess.

Comment on lines -6 to -16
fixBy
(all
F
(fun (type) (type))
(fun
(fun (all Q (type) (fun [ F Q ] Q)) (all Q (type) (fun [ F Q ] Q)))
(fun
(all Q (type) (fun [ F Q ] [ F Q ])) (all Q (type) (fun [ F Q ] Q))
)
)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixBy is gone.

@effectfully effectfully merged commit 16be7da into master May 6, 2024
6 checks passed
@effectfully effectfully deleted the effectfully/plutus-ir/do-not-generate-fixBy-if-you-do-not-need-to branch May 6, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement No Changelog Required Add this to skip the Changelog Check Plutus IR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants