Skip to content
Merged
Changes from 1 commit
Commits
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
7 changes: 2 additions & 5 deletions .github/workflows/sycl-linux-run-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -284,11 +284,8 @@ jobs:
echo "opts=$CMAKE_EXTRA_ARGS" >> $GITHUB_OUTPUT
else
if [ "${{ contains(inputs.target_devices, 'ext_oneapi_hip') }}" == "true" ]; 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 am of the opinion that we should move all the runner specific code to the caller instead. For post-commit, in sycl-post-commit: https://github.com/intel/llvm/blob/sycl/.github/workflows/sycl-post-commit.yml#L85, just like what we do for docker image options. In this case, we can pass CMake build options to sycl-link-run-tests using extra_cmake_args.

Copy link
Contributor Author

@sarnex sarnex Nov 13, 2024

Choose a reason for hiding this comment

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

This isn't really runner specific code, it's target device specific code. Any time this workflow is called targeting HIP we will need this to run. Another downside of moving it up is we would have to duplicate this code in the pre and postcommit callsites, or make another workflow that implements this script in call it in both places.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thoughts were the same as @uditagarwal97 suggested. I'm not sure if that duplication is enough to convince me to do things differently, but I don't have strong opinion about it anyway.

Copy link
Contributor Author

@sarnex sarnex Nov 13, 2024

Choose a reason for hiding this comment

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

Looking now, even if we wanted to do it at the callsite, I don't think it will work if I understand what's happening correctly. So like here or below here, I don't think this runs on the runner that will run the test, I think it runs on the machine running the larger workflow. We need this command to run on the machine with the AMD GPU, so I'm not sure how we could move it up a layer. Also, since we need to run sycl-ls, we need the toolchain downloaded, which I don't think is available at the callsites either.

if [ "${{ runner.name }}" == "cp-amd-runner" ]; then
echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH="gfx1030"' >> $GITHUB_OUTPUT
else
echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH="gfx1031"' >> $GITHUB_OUTPUT
fi
amd_arch="$( env ONEAPI_DEVICE_SELECTOR=hip:gpu sycl-ls --verbose 2>&1 | grep 'Architecture:' | sed 's|\s*Architecture: amd_gpu_||g' )"
echo 'opts=-DHIP_PLATFORM="AMD" -DAMD_ARCH='$amd_arch'' >> $GITHUB_OUTPUT
else
echo 'opts=' >> $GITHUB_OUTPUT
fi
Expand Down
Loading