Skip to content

[CI] Fix AMDGPU arch flag for Codeplay runner #16053

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 12, 2024
Merged

[CI] Fix AMDGPU arch flag for Codeplay runner #16053

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .github/workflows/sycl-linux-run-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,11 @@ jobs:
echo "opts=$CMAKE_EXTRA_ARGS" >> $GITHUB_OUTPUT
else
if [ "${{ contains(inputs.target_devices, 'ext_oneapi_hip') }}" == "true" ]; then
echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH="gfx1031"' >> $GITHUB_OUTPUT
if [ "${{ runner.name }}" == "cp-amd-runner" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hesitant about hard coding the runner name here. Just like we have extra_lit_opts, can we have another matrix input, something like extra_lit_build_opts, for passing runner specific build options? Or perhaps we can reuse inputs.extra_cmake_args?

Copy link
Contributor Author

@sarnex sarnex Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could, but wouldn't we have to have the same hardcoded runner name check at the place where we decide what to pass for extra_lit_opts? There are two runners and they need different values for this variable. I think the real solution is running sycl-ls and grepping the output for the architecture, but I didn't want to overengineer it if the solution in this PR is acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, running sycl-ls and grepping the architecture type would be an ideal solution. I don't have strong objection to the changes in this PR, so I'll approve, but can you also create a Github issue to "use sycl-ls for determining the runner architecture type"? I can implement that once I get some free cycles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, i can own it actually, seems at least somewhat interesting.

#16057

echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH="gfx1030"' >> $GITHUB_OUTPUT
else
echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH="gfx1031"' >> $GITHUB_OUTPUT
fi
else
echo 'opts=' >> $GITHUB_OUTPUT
fi
Expand Down
Loading