feat: initial support for Clang HIP #2045
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2045 +/- ##
===========================================
+ Coverage 30.91% 43.09% +12.18%
===========================================
Files 53 53
Lines 20112 20576 +464
Branches 9755 9930 +175
===========================================
+ Hits 6217 8868 +2651
+ Misses 7922 7893 -29
+ Partials 5973 3815 -2158 ☔ View full report in Codecov by Sentry. |
|
An implementation questions: As mentioned earlier, I want to include changes in the However, this presents a problem: the location of those files can be determined by both the environment variable and command-line arguments, but there doesn't seem to be a function where I have access to both of them. Ideally I want to add those files to While I do have access to Would it be accepted if |
|
Hi @sylvestre or @Xuanwo, could one of you provide some advice on how my problem above might be solved? |
|
HIP_DEVICE_LIB_PATH and HIP_PATH are standard and expected env variables in this ecosystem? Sorry, i don't know much here |
GZGavinZhao
force-pushed
the
hip
branch
4 times, most recently
from
07f17ac
to
68b2b45
Compare
|
Sorry, please ignore my previous comment. I figured the current implementation is good enough. Feature-wise this is complete. I just need to write up docs and add E2E tests in |
GZGavinZhao
force-pushed
the
hip
branch
2 times, most recently
from
d9cc616
to
e4debdc
Compare
| exe: T, | ||
| input: &str, | ||
| output: &str, | ||
| archs: &Vec<String>, |
There was a problem hiding this comment.
I'll leave it Vec<String> instead of Vec<OsString> for now because I had a little trouble figuring out how to concat two OsString.
GZGavinZhao
force-pushed
the
hip
branch
5 times, most recently
from
a11638a
to
1d9a69b
Compare
|
I still need to write up docs, but I would appreciate feedback on whether you guys are okay with the logic to find and hash bitcode libraries (these changes are in |
src/compiler/c.rs
Outdated
| // too much to handle on our side so we just hash every bitcode library in that | ||
| // directory. | ||
| if args.language == Language::Hip { | ||
| let mut rocm_path_arg: Option<PathBuf> = None; |
There was a problem hiding this comment.
please avoid "let mut" and the following
move the work into a function
There was a problem hiding this comment.
Fixed, please check if the current version is acceptable.
src/compiler/c.rs
Outdated
| let hip_device_lib_path_env: Option<PathBuf> = | ||
| extract_rocm_env("HIP_DEVICE_LIB_PATH"); | ||
|
|
||
| let hip_device_lib_path: PathBuf = hip_device_lib_path_arg |
There was a problem hiding this comment.
please move this into a function
src/compiler/c.rs
Outdated
| // too much to handle on our side so we just hash every bitcode library in that | ||
| // directory. | ||
| if args.language == Language::Hip { | ||
| let extract_rocm_arg = |flag: &str| { |
There was a problem hiding this comment.
actually, would it be possible to move all this block into a function?
There was a problem hiding this comment.
Yes, I'm thinking of something like fn search_hip_device_libs(args: &ParsedArguments, env_vars: &[(OsString, OsString)]) -> Vec<PathBuf>. Where shall I put this function, in src/util.rs?
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
Signed-off-by: Gavin Zhao <git@gzgz.dev>
|
Thanks! |
Fixes #2044.
Still needs to distinguished between different ROCm versions, due to implicit dependencies that may be introduced byNo need to concern about header files, but for the bitcode libraries I'm still thinking.$HIP_DEVICE_LIB_PATH/*.bcand header files located at$HIP_PATH/include/hip/**/*.hI currently have an experimental support for detecting and adding the bitcode libraries into
extra_hash_files. If the implementation is undesired, then my idea is to just ignore bitcode libraries and explicitly mention in the docs to add the bitcode libraries toSCCACHE_EXTRA_FILESif the user is worried about erroneous cache hits.