port class 'dynamic_loader' to Windows #2278
Merged
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
pfultz2
reviewed
Oct 3, 2023
src/CMakeLists.txt
Outdated
| @@ -104,6 +103,11 @@ add_library(migraphx | |||
| value.cpp | |||
| verify_args.cpp | |||
| ) | |||
| if(WIN32) | |||
| target_sources(migraphx PRIVATE dynamic_loader_win32.cpp) | |||
There was a problem hiding this comment.
Put this in the same file.
Codecov Report
@@ Coverage Diff @@
## develop #2278 +/- ##
========================================
Coverage 91.45% 91.45%
========================================
Files 433 433
Lines 16174 16174
========================================
Hits 14792 14792
Misses 1382 1382
|
pfultz2
reviewed
Oct 3, 2023
src/dynamic_loader_win32.cpp
Outdated
| std::to_string(GetLastError()) + ")"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You need to add a move constructor and delete the copy constructor, and add an assignment operator.
pfultz2
reviewed
Oct 3, 2023
src/dynamic_loader_win32.cpp
Outdated
| } | ||
|
|
||
| HMODULE handle = nullptr; | ||
| std::shared_ptr<tmp_dir> temp = nullptr; |
There was a problem hiding this comment.
Since the class is not copyable, then there is no reason to use shared_ptr here.
pfultz2
reviewed
Oct 3, 2023
| @@ -46,6 +47,7 @@ struct MIGRAPHX_EXPORT dynamic_loader | |||
| static fs::path path(void* address); | |||
|
|
|||
| static optional<dynamic_loader> try_load(const fs::path& p); | |||
| #endif | |||
There was a problem hiding this comment.
Why is this disabled on windows?
There was a problem hiding this comment.
For some reason, on Windows, static analysis is showing the catch part of the function is unreachable code. I made it available on Windows because it looks like a false-positive case.
This build is OK for merge ✅ |
🔴bert_base_cased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output🔴distilgpt2_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
pfultz2
reviewed
Oct 8, 2023
| std::to_string(GetLastError()) + ")"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Make sure to delete the copy constructor and add a an assignment operator.
pfultz2
approved these changes
Oct 10, 2023
causten
requested changes
Oct 11, 2023
causten
approved these changes
Oct 11, 2023
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.
No description provided.