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

[Optimizer] Refactor Optimizer lib into {Graph,IR}Optimizer libs #3165

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@jfix71
Copy link
Contributor

commented Jun 24, 2019

Summary: Allow for more precise dependences on libs. Most places rely on either the Graph or IR Optimizer, not both. Related to #3161.

Test Plan: All tests pass.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Is this going to be purely change in the open source only?

@jfix71

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

No, I'll make similar changes in fbcode. I tend to always work open-source first and make fbcode changes after the open source ones have been approved.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Have not noticed moving cpps around and thought it's just CMake changes, yeah, sounds good.

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@jfix71 For some reason, DEBUG build shows a lot of duplicate symbols errors.

@rdzhabarov

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@jfix71 For some reason, DEBUG builds shows a lot of duplicate symbols errors.

Those are the only one which uses open cl, seems like it's tests/OCLTest

@jfix71

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

Working on fixing. Not sure what's going on -- I need to include OpenCLBackend in OCLTest on my Mac for it to compile, but that causes duplicate symbols on Linux. But if I don't include OpenCLBackend in OCLTest then it compiles on Linux but my Mac has missing symbols 🙁

@opti-mix

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Working on fixing. Not sure what's going on -- I need to include OpenCLBackend in OCLTest on my Mac for it to compile, but that causes duplicate symbols on Linux. But if I don't include OpenCLBackend in OCLTest then it compiles on Linux but my Mac has missing symbols 🙁

I think it is due to our dynamic backends registrations and whole-archive options, which work differently on Linux and MacOS. @rdzhabarov may have some war stories about it ;-)

@jfix71 jfix71 force-pushed the jfix71:refactor_optimizer branch 3 times, most recently from fdae793 to 26672e5 Jun 25, 2019

@jfix71 jfix71 force-pushed the jfix71:refactor_optimizer branch from 26672e5 to 8ae9b8e Jun 25, 2019

@facebook-github-bot
Copy link

left a comment

@jfix71 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jfix71

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

OCLTest works now when I include Backends instead of OpenCLBackend. I also moved/updated all the headers.

@opti-mix
Copy link
Contributor

left a comment

Looking good! Thanks!

@rdzhabarov
Copy link
Contributor

left a comment

Nice!

@jfix71 jfix71 deleted the jfix71:refactor_optimizer branch Jun 26, 2019

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jun 26, 2019

@jfix71 merged this pull request in 3ebf837.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.