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

Add EqualityMapper #74

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Nov 15, 2021

Added an EqualityMapper similar to the one in inducer/pytato#166.

This seems a bit slower than the current version (benchmarked against inducer/pytential#121):
it went from 5.3s-ish to 6.6s-ish.

Fixes #73.

@inducer
Copy link
Owner

inducer commented Apr 28, 2022

@kaushikcfd Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

@alexfikl
Copy link
Collaborator Author

@kaushikcfd Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

For what it's worth, the loopy branch is up to date and passing tests locally, so it shouldn't be too hard to clean this up.

@kaushikcfd
Copy link
Collaborator

Is this worth pursuing given that compile times are suffering because of expression traversal complexity in pymbolic?

I think this is useful. IIUC, this PR quite significantly brings down the complexity of the expression comparison. If the overheads of this approach (creating a new mapper for every comparison) are under-control, I would love to get rid of hacks such as: https://github.com/kaushikcfd/pymbolic/blob/a697d47cd53e9902abc49f98d67921b466c992c2/pymbolic/mapper/__init__.py#L213.

@kaushikcfd
Copy link
Collaborator

However, I wonder how the loopy CI failures could be fixed? By creating a derived version of every pymbolic expression type in loopy so that they can use loopy-specific EqualityMapper?

@alexfikl
Copy link
Collaborator Author

However, I wonder how the loopy CI failures could be fixed? By creating a derived version of every pymbolic expression type in loopy so that they can use loopy-specific EqualityMapper?

It should work with the modified branches. I imagine it needs some work in ci-support.sh to clone the correct repo and branch (instead of hardcoding to inducer/loopy@main).

@kaushikcfd
Copy link
Collaborator

Oops had missed the branch in the description. I think those will work fine. Thanks!

@inducer
Copy link
Owner

inducer commented Apr 29, 2022

I would love to get rid of hacks such as:

Whoa. I had missed that during review. That is pretty gross.

@inducer
Copy link
Owner

inducer commented Apr 29, 2022

@inducer
Copy link
Owner

inducer commented Apr 29, 2022

I need to do some measuring.

@alexfikl alexfikl mentioned this pull request Apr 30, 2022
@alexfikl alexfikl force-pushed the equality-mapper branch 3 times, most recently from 2038873 to 6073162 Compare May 4, 2022 14:09
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.

Equality comparison of DAGs can have exponential complexity
3 participants