Fix a collection of issues related to Windows/MSVC #45
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- These may be defined by windows.h and cause `std::min/max` to break.
- Since we require C++11 now, these are no longer needed. - This also avoids the problem of MSVC not correctly defining __cplusplus.
- Unused __constant__ variables could cause the same code-bloat problems as __device__ variables (though they are less common).
- Can be re-enabled by defining JITIFY_ENABLE_EMBEDDED_FILES to 1. - This is a rarely-used feature and only works with GCC on Linux. Disabling it avoids having a hard dependency dlfcn, which is not always available on Windows. It also avoids needing to link with -ldl.
- The global-namespace versions of these are actually built into NVRTC. Providing our own duplicate definitions caused errors in MSVC because they conflicted with the NVRTC-provided definitions. We only need to provide the std:: versions of them.
- PTX name mangling always follows the Itanium64 ABI scheme, even under MSVC. This means we can't use the platform-specific demangler for creating the map of __constant__/__device__ symbols. - The nvrtcGetLoweredName function would work except that it requires all names to have been added via nvrtcAddNameExpression _before_ compilation, which is not feasible/convenient with our API. - This commit adds a simple custom demangler that supports nested namespace-scope variable names in the PTX (Itanium64) mangling format. - Only namespace-scope variable names need to be supported because __device__/__constant__ variables are only allowed at namespace-scope or function scope (if static), and the latter cannot be directly addressed from outside the function.
- This used to fail on Windows under MSVC but should work now.
Closed
maddyscientist
approved these changes
Feb 28, 2020
There was a problem hiding this comment.
Approving. This looks solid. I suggest we hold off merging this until @mondus confirms that this pull fixes his outstanding issues.
- Required to compile under Visual Studio 2015.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes issues found in #43 (along with a few other minor things that were noticed along the way).
The main fixes relate to: min/max macros, __cplusplus macro, dlfcn dependency, incompatible size_t definition, and incorrect name demangling.
Full details are in the commit messages.