Skip to content

Remove symbol export annotations in THC/generic/*.cu#11367

Closed
peterjc123 wants to merge 6 commits intopytorch:masterfrom
peterjc123:cleanup_fix
Closed

Remove symbol export annotations in THC/generic/*.cu#11367
peterjc123 wants to merge 6 commits intopytorch:masterfrom
peterjc123:cleanup_fix

Conversation

@peterjc123
Copy link
Collaborator

We use these annotations during function declarations, not definitions. See the description of compiler error C2491 for more details.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@Yangqing
Copy link
Contributor

Yangqing commented Sep 7, 2018

@peterjc123 since THC_API also includes the THC_EXTERN (aka extern "C") - maybe we need to change it to THC_EXTERN instead of removing it?

Copy link
Contributor

@Yangqing Yangqing left a comment

Choose a reason for hiding this comment

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

(see comment above)

@peterjc123
Copy link
Collaborator Author

@Yangqing Sorry, I don't quite get the reasons to change them to THC_EXTERN. I removed them because these functions are already annotated as THC_API in the header files. So the annotations in the source file are duplicated and sometimes cause compiling errors like C2491. Current build failed due to some missing function declarations.

@peterjc123
Copy link
Collaborator Author

The missing of the export symbols sort, sortKeyValueInplace and topk is caused by the broken reference link. It seems these header files have been referenced in THC/THCTensorMath.h and then THC/THC.h. But no file has the reference to THC/THC.h during the build. May I know how to fix this reference link?

@Yangqing
Copy link
Contributor

Yangqing commented Sep 7, 2018

Ah makes sense, I'll leave the symbol issues for the original TH authors to deal with then...

@Yangqing Yangqing dismissed their stale review September 7, 2018 17:43

(dismissed - issue is otherwise)

@peterjc123
Copy link
Collaborator Author

cc. @ezyang @fmassa

@yf225
Copy link
Contributor

yf225 commented Sep 11, 2018

@peterjc123 probably need a rebase

@ezyang @fmassa Any suggestions for this PR?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@peterjc123
Copy link
Collaborator Author

@ezyang @soumith Could you please take a look and merge this PR, please?

zdevito pushed a commit to zdevito/ATen that referenced this pull request Sep 17, 2018
Summary:
We use these annotations during function declarations, not definitions. See the description of compiler error [C2491](https://msdn.microsoft.com/en-us/library/62688esh.aspx) for more details.
Pull Request resolved: pytorch/pytorch#11367

Reviewed By: ezyang

Differential Revision: D9697923

Pulled By: orionr

fbshipit-source-id: 1e539c02957851386f887e6d0510ce83117a1695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants