This is an archive of the discontinued LLVM Phabricator instance.

[HIP] Fix ROCm detection
ClosedPublic

Authored by yaxunl on Mar 18 2021, 7:24 AM.

Details

Summary

ROCm has changed installation path to /opt/rocm-{release}. Add detection
for that. Also support ROCM_PATH environment variable.

Diff Detail

Event Timeline

yaxunl created this revision.Mar 18 2021, 7:24 AM
yaxunl requested review of this revision.Mar 18 2021, 7:24 AM
tra added inline comments.Mar 18 2021, 11:08 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
271

This will rank rocm-3.9 higher than rocm-3.10. I think you do need to extract and compare the version.

yaxunl marked an inline comment as done.Mar 19 2021, 8:25 AM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/AMDGPU.cpp
271

Good catch. Will fix it.

yaxunl updated this revision to Diff 331889.Mar 19 2021, 8:31 AM
yaxunl marked an inline comment as done.

fix version comparison

tra added inline comments.Mar 19 2021, 10:08 AM
clang/lib/Driver/ToolChains/AMDGPU.cpp
267–268

You don't need StringRef here. VerStr.rfind() will do.

269

This only deals with a single . in the version. It's likely that we will have to deal with a patch release at some point, so you may want to replace all instances.

yaxunl marked 2 inline comments as done.Mar 20 2021, 6:23 AM
yaxunl added inline comments.
clang/lib/Driver/ToolChains/AMDGPU.cpp
267–268

will fix

269

The ROCm directory follows rocm-{major}.{minor}.{subMinor}[-{build}] format, where major, minor and path (subMinor) are delimited by '.', only the optional build number is delimited by '-'. I have a comment about that at line 266.

yaxunl updated this revision to Diff 332104.Mar 20 2021, 7:45 AM
yaxunl marked 2 inline comments as done.

revised by Artem's comments

tra accepted this revision.Mar 22 2021, 11:55 AM
tra added inline comments.
clang/lib/Driver/ToolChains/AMDGPU.cpp
269

Sorry, my mistake. Two of them. I've overlooked that the version had dash in it, but we've actually been replacing an underscore.
And then I've missed the commend and assumed that all components are separated with the underscore.

The code looks good now.

This revision is now accepted and ready to land.Mar 22 2021, 11:55 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2021, 1:10 PM