- Notifications
You must be signed in to change notification settings - Fork 15.3k
[mlir][spirv] Add mlir-spirv-tests CI to run for mlir-spv target tests #152124
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
Conversation
| @llvm/pr-subscribers-github-workflow Author: Davide Grohmann (davidegrohmann) ChangesFull diff: https://github.com/llvm/llvm-project/pull/152124.diff 1 Files Affected:
diff --git a/.github/workflows/spirv-tests.yml b/.github/workflows/spirv-tests.yml index f15ca1cb64ba5..a2bb64d400bbf 100644 --- a/.github/workflows/spirv-tests.yml +++ b/.github/workflows/spirv-tests.yml @@ -23,7 +23,7 @@ jobs: name: Test SPIR-V uses: ./.github/workflows/llvm-project-tests.yml with: - build_target: check-llvm-codegen-spirv + build_target: check-llvm-codegen-spirv check-mlir projects: - extra_cmake_args: '-DLLVM_TARGETS_TO_BUILD="SPIRV" -DLLVM_INCLUDE_SPIRV_TOOLS_TESTS=ON' + extra_cmake_args: '-DLLVM_TARGETS_TO_BUILD="X86;SPIRV" -DLLVM_INCLUDE_SPIRV_TOOLS_TESTS=ON' os_list: '["ubuntu-24.04"]' |
28e45e5 to 2acd645 Compare
kuhar left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would be better to bifurcate spirv-tests.yml and have a separate version for mlir-spirv. It's very rare that we have PRs that affect both implementations, so having them separate could save us some CI time.
2acd645 to 22ca55f Compare
Yes it is likely better. This is now done. |
This should execute also the MLIR SPIRV Target tests which require the SPIRV-Tools validation Signed-off-by: Davide Grohmann <davide.grohmann@arm.com> Change-Id: Ied323152e36b95d6f21ed7d4ce4f5f813f280f17
22ca55f to 0301179 Compare Signed-off-by: Davide Grohmann <davide.grohmann@arm.com> Change-Id: I52a5fcf79aba2c272a2ac6a4dc2c393a02b398d9
- add SPIRV dialect definition and implementation - add serialization/deserialization implementation Signed-off-by: Davide Grohmann <davide.grohmann@arm.com> Change-Id: Ib63045fec041ba7a2d52b527589c30157ed56258
Signed-off-by: Davide Grohmann <davide.grohmann@arm.com> Change-Id: I9919a359c8fbefbc5dd030f9a19c7a83fab12682
| I'm going to merge once we confirm that the new check works as intended: https://github.com/llvm/llvm-project/actions/runs/16807047739/job/47602153053?pr=152124 |
| MLIR SPIR-V tests seem to take ~1h (https://github.com/llvm/llvm-project/actions/workflows/mlir-spirv-tests.yml) compared to regular checks that finish in ~5-10min. I wonder if there is any reason for that and whether we can bring the time down? |
I can have a look. Can you point me to the workflows scripts for the regular checks so I can compare those with the mlir spir-v tests script? |
| Probably no build cache? When I built this configuration locally, a lot of time was spent during spirv-tools checkout. I wonder if we can optimize how this gets cloned, e.g., |
I believe it's "CI Checks" (https://github.com/llvm/llvm-project/actions/workflows/premerge.yaml), script: https://github.com/llvm/llvm-project/blob/main/.github/workflows/premerge.yaml |
| I have also noticed that auto-merge doesn't wait for this job: #153440 We also should address that if deemed necessary. Not sure if this PR is the best place to track those issues. |
Those checks run on powerful self hosted runners (64 threads) and have good build caching. This workflow runs on the free four core Github runners and Github build caching is generally pretty poor. It is going to be a lot slower.
Enabling auto-merge requires making the check required. Required checks cannot have path dependencies. I also doubt the community is interested in making this check required. |
That makes sense, thanks for investigating.
In the hindsight, I agree we probably don't need this check as required, it just took me by surprise, and was quite busy that day, so I wasn’t sure what to do with it :) |
| What is the plan then? Should we switch to use the more powerful runners? |
This workflow should probably stay as it is. The |
This should execute also the MLIR SPIRV Target tests which require the SPIRV-Tools validator