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.

Sign up for GitHub

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

Jump to bottom

Add commuteConst to simplifier pass. #5360

Merged
merged 9 commits into from
Jun 5, 2023
Merged

Add commuteConst to simplifier pass. #5360

merged 9 commits into from
Jun 5, 2023

Conversation

thealmarty
Copy link
Contributor

@thealmarty thealmarty commented May 25, 2023 •
edited
Loading

Doing this in PIR in case we want to do CSE in PIR.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch from 562d229 to 78e11c9 Compare May 25, 2023 17:52
@thealmarty thealmarty changed the title Add commuteEquals to simplifier pass. Add commuteConst to simplifier pass. May 25, 2023
@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch 2 times, most recently from caa5707 to 971003f Compare May 25, 2023 18:13
@thealmarty thealmarty requested review from michaelpj and zliu41 May 25, 2023 18:17
@thealmarty thealmarty marked this pull request as ready for review May 25, 2023 18:17
@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch from 971003f to b6d90f6 Compare May 26, 2023 14:45
@thealmarty thealmarty requested a review from zliu41 May 26, 2023 17:26
@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch from dd06e77 to 1967033 Compare May 26, 2023 17:30
michaelpj
michaelpj reviewed May 30, 2023
View reviewed changes
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs Outdated Show resolved Hide resolved
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs Outdated Show resolved Hide resolved
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs Outdated Show resolved Hide resolved
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs
SerialiseData -> False
MkPairData -> False
MkNilData -> False
MkNilPairData -> False
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure, but I think given we're listing some exceptions, this could reasonably have a _ -> False case.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer the catch-all case. It actually makes it easier to catch mistakes.

Copy link
Contributor Author

@thealmarty thealmarty May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What I'm trying to do is to force us to add a case here whenever we add a new DefaultFun. Otherwise, any new builtins will just be false, because we're unlikely to remember this unless the compiler reminds us. Of course, not remembering is not the end of the world because we just won't commute, but I think it makes sense to add any upcoming commutative functions in here so that the optimization will apply.

Also, I generally don't like catchall for this reason - things just work, but maybe not as you intended!

plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs Show resolved Hide resolved
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs Outdated Show resolved Hide resolved
plutus-core/plutus-ir/src/PlutusIR/Transform/CommuteFnWithConst.hs

commuteFnWithConst :: forall tyname name uni fun a. Typeable fun =>
Term tyname name uni fun a -> Term tyname name uni fun a
commuteFnWithConst = case eqT @fun @DefaultFun of
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we decided we were going to take it as a parameter like InlineHints?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this at all: it just means it can't work for anything except DefaultFun.

Copy link
Member

Choose a reason for hiding this comment

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

I thought we decided we were going to take it as a parameter like InlineHints?

I think that's what you proposed rather than what we decided 😂

That would require adding additional parameters to a number of functions. It does make it work with other funs than DefaultFun, but given we aren't planning to have anything other than DefaultFun anytime soon, I'm inclined to say not to optimize prematurely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's in this PR too. Although I guess that's not merged. I'm fine with either way really. I have more preference for things to work similarly in the same code base. So it comes down to whether you want to change your #5337 Ziyang!

@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch from 1967033 to 9a93b5c Compare May 30, 2023 16:00
@thealmarty thealmarty force-pushed the thealmarty/plt-1187-commuteEquals branch from 9a93b5c to 4d3c797 Compare May 31, 2023 15:07
@thealmarty thealmarty requested a review from michaelpj June 5, 2023 15:01
@zliu41 zliu41 merged commit cce5fbf into master Jun 5, 2023
@zliu41 zliu41 deleted the thealmarty/plt-1187-commuteEquals branch June 5, 2023 16:56
v0d1ch pushed a commit to v0d1ch/plutus that referenced this pull request Dec 6, 2024
@thealmarty @v0d1ch
Add commuteConst to simplifier pass. (IntersectMBO#5360)
041ba87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zliu41 zliu41 zliu41 approved these changes

@michaelpj michaelpj Awaiting requested review from michaelpj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thealmarty @michaelpj @zliu41