Skip to content

Add windows-amd and windows-nvidia GitHub workflows for the two new machines #313

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

Merged
merged 12 commits into from
Aug 6, 2025

Conversation

Icohedron
Copy link
Contributor

@Icohedron Icohedron commented Aug 5, 2025

This PR duplicates the existing windows-intel github workflows to support the new windows-amd and windows-nvidia machines.

windows-amd and windows-nvidia have also been added to the pr-matrix workflow SKUs for the Exec-Tests-Windows job.

The WARP workflow has been changed to run solely on the AMD machine only.

@damyanp
Copy link
Collaborator

damyanp commented Aug 5, 2025

The duplication here concerns me. Why can we use a matrix for the PR jobs, but not the others?

@Icohedron
Copy link
Contributor Author

The duplication here concerns me. Why can we use a matrix for the PR jobs, but not the others?

IIRC, @llvm-beanz said it was to have individual badges / status icons for each job.
It's the same reason the windows-intel workflows are separated into separate files instead of being its own matrix.

@Icohedron
Copy link
Contributor Author

I'm not sure we need to duplicate the WARP workflows though. Wouldn't the WARP test result be expected to be the same across all machines? We could designate just one machine to running the WARP tests.

@damyanp
Copy link
Collaborator

damyanp commented Aug 5, 2025

The duplication here concerns me. Why can we use a matrix for the PR jobs, but not the others?

IIRC, @llvm-beanz said it was to have individual badges / status icons for each job. It's the same reason the windows-intel workflows are separated into separate files instead of being its own matrix.

Ah, I see, that's an unfortunate limitation from github.

But it does remind me that you should also update README.md to reference these badges.

@damyanp
Copy link
Collaborator

damyanp commented Aug 5, 2025

I'm not sure we need to duplicate the WARP workflows though. Wouldn't the WARP test result be expected to be the same across all machines? We could designate just one machine to running the WARP tests.

They'd certainly be expected to have the same results.

As the same time, if they gave different results then that'd be good to know!

@damyanp
Copy link
Collaborator

damyanp commented Aug 5, 2025

I'm not sure we need to duplicate the WARP workflows though. Wouldn't the WARP test result be expected to be the same across all machines? We could designate just one machine to running the WARP tests.

They'd certainly be expected to have the same results.

As the same time, if they gave different results then that'd be good to know!

Maybe could consider a consolidated WARP workflow that runs on all the machines?

@llvm-beanz
Copy link
Collaborator

As the same time, if they gave different results then that'd be good to know!

I think if we wanted to run the WARP tests on different underlying hardware we should do that only in the periodic builds not the pre-merge builds. We could alternatively just allow the WARP scheduled builds to use any windows builder which will cycle it through based on machine availability. That might be good enough to provide some coverage without slowing down our builds too much.

Comment on lines 11 to 13
* CPU: Intel Xeon <I'll figure out the model>
* GPU: Intel UHD Graphics 630
* RAM: <I should figure this out too>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting on @llvm-beanz to provide the specs for the intel machine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble remoting into the machine. Can you just leave blank lines? I'll hunt it down and update after this PR merges.

strategy:
fail-fast: false
matrix:
SKU: [windows-amd]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to pick a SKU randomly or based on availability, so I picked the AMD machine because it has the better CPU and the most RAM to work with between the NVIDIA and AMD machines.

I'm not sure yet what the specs of the Intel machine are, nor if we would would want to keep running the Warp tests on it because it's not a lab machine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems reasonable. The Intel machine I have is old and slow, so doing less on it is probably good.

Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

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

All looks good once the bots go green.

@Icohedron
Copy link
Contributor Author

All looks good once the bots go green.

There's a lot of red :(

@Icohedron Icohedron self-assigned this Aug 6, 2025
@Icohedron Icohedron moved this to Active in HLSL Support Aug 6, 2025
These machines currently have execution test failures that need to be
resolved before they can be part of PR pre-merge checks.
@Icohedron Icohedron merged commit 266bdd7 into llvm:main Aug 6, 2025
8 of 9 checks passed
@github-project-automation github-project-automation bot moved this from Active to Closed in HLSL Support Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants