Skip to content

Expose stream executor namespace in cmake shared object. #19415

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
Merged
gunan merged 6 commits into from
May 23, 2018
Merged

Expose stream executor namespace in cmake shared object. #19415

gunan merged 6 commits into from
May 23, 2018

Conversation

@gunan
Copy link
Contributor

@gunan gunan commented May 20, 2018

No description provided.

@gunan gunan changed the title Instead of expose stream executor namespace in cmake shared object. Expose stream executor namespace in cmake shared object. May 20, 2018
@gunan
Copy link
Contributor Author

gunan commented May 20, 2018

Testing here:
http://ci.tensorflow.org/view/TF%20pull%20requests/job/tf-pr-win-cmake-gpu/29/

@gunan
Copy link
Contributor Author

gunan commented May 20, 2018

Another try here
http://ci.tensorflow.org/view/TF%20pull%20requests/job/tf-pr-win-cmake-gpu/30/

@gunan
Remove "se" namespace.
17f610d 
The one we are interested in is tensorflow::se and it is covered by a previous regex.
@gunan
Copy link
Contributor Author

gunan commented May 20, 2018

Yet another try:
http://ci.tensorflow.org/job/tf-pr-win-cmake-gpu/31/

mrry
mrry reviewed May 21, 2018
View reviewed changes
tensorflow/contrib/cmake/tools/create_def_file.py
r"functor::|"
r"\?nsync_|"
r"perftools::gputools")
r"stream_executor::")
Copy link
Contributor

@mrry mrry May 21, 2018

Choose a reason for hiding this comment

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

This looks like the right fix, but it seems we're once again hitting the 2**16 exported symbols limit. Perhaps stream_executor::* contains far more symbols that perftools::gputools ever did?

I don't know anything about the structure of that namespace, but is it possible we could use a narrower regex to get the symbols we need here? Ideally we'd have some sort of explicit public or private/impl subnamespace that we could include/exclude as possible, but I'm not sure if such a thing exists...

@case540 case540 self-assigned this May 21, 2018
@gunan
Copy link
Contributor Author

gunan commented May 23, 2018

I think we now got around the symbol limit issue, however now I am seeing this:
http://ci.tensorflow.org/view/TF%20pull%20requests/job/tf-pr-win-cmake-gpu/34/console

C:\tf_jenkins\workspace\tf-pr-win-cmake-gpu\cmake_build\Release\contrib_image_sirds_ops_gen_python.exe : fatal error LNK1169: one or more multiply defined symbols found [C:\tf_jenkins\workspace\tf-pr-win-cmake-gpu\cmake_build\contrib_image_sirds_ops_gen_python.vcxproj]

@gunan
Copy link
Contributor Author

gunan commented May 23, 2018

We now have a successful build!
http://ci.tensorflow.org/view/TF%20pull%20requests/job/tf-pr-win-cmake-gpu/38/

mrry
mrry approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@mrry mrry left a comment

Choose a reason for hiding this comment

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

Hooray!

@gunan gunan merged commit a4a6bab into tensorflow:master May 23, 2018
@gunan gunan deleted the cmake branch May 23, 2018 22:49
@bstriner bstriner mentioned this pull request Jun 5, 2018
Closed
@fo40225 fo40225 mentioned this pull request Aug 12, 2018
Open
@fo40225 fo40225 mentioned this pull request Jun 17, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mrry mrry mrry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@case540 case540

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gunan @mrry @case540 @googlebot