-
Notifications
You must be signed in to change notification settings - Fork 86
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
Remove the hard coded path in MIGRAPHX_CXX_COMPILER #3377
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3377 +/- ##
========================================
Coverage 92.02% 92.02%
========================================
Files 509 509
Lines 21014 21014
========================================
Hits 19339 19339
Misses 1675 1675 ☔ View full report in Codecov by Sentry. |
This should still use the absolute path so we can be sure its using the same compiler the developer set migraphx to build with. I dont see any reason why this should be changed either as it doesnt affect any users. |
As part of our efforts to make ROCm relocatable , these hard coded paths need to be removed. |
src/CMakeLists.txt
Outdated
set(COMPILER_NAME ${GET_COMPILER_NAME}) | ||
endif() | ||
endif()# end of CPU/GPU check | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be any changes to cmake. The compiler_src.hpp should be updated to only use MIGRAPHX_CXX_COMPILER
macro when its not windows:
struct MIGRAPHX_EXPORT src_compiler
{
#ifdef _WIN32
fs::path compiler = MIGRAPHX_CXX_COMPILER;
#else
fs::path compiler = "c++";
#endif
std::vector<std::string> flags = {};
fs::path output = {};
fs::path launcher = {};
std::string out_ext = ".o";
std::function<fs::path(fs::path)> process = nullptr;
std::vector<char> compile(const std::vector<src_file>& srcs) const;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pfultz2
Updated the PR as suggested
This build is not recommended to merge 🔴 |
🔴bert_large_uncased_fp16: FAILED: MIGraphX is not within tolerance - check verbose output |
amdclang++/amdclang can be invoked without providing absolute path, since update-alternatives command has been run for these binaries This will also help to remove the hard coded paths in cmake target files for these compilers
186d303
to
f34d90c
Compare
amdclang++/amdclang can be invoked without providing absolute path, since update-alternatives command has been run for these binaries This will also help to remove the hard coded paths in cmake target files for these compilers