Add pre-merge workflow for HLSL testing #122184
Conversation
This adds a workflow for running HLSL tests on PRs that modify HLSL and DirectX code.
|
@llvm/pr-subscribers-github-workflow Author: Chris B (llvm-beanz) ChangesThis adds a workflow for running HLSL tests on PRs that modify HLSL and DirectX code. Full diff: https://github.com/llvm/llvm-project/pull/122184.diff 2 Files Affected:
diff --git a/.github/workflows/hlsl-macos.yaml b/.github/workflows/hlsl-macos.yaml
new file mode 100644
index 00000000000000..f63888a8b9beff
--- /dev/null
+++ b/.github/workflows/hlsl-macos.yaml
@@ -0,0 +1,26 @@
+name: HLSL Tests - macOS
+
+permissions:
+ contents: read
+ checks: write
+
+on:
+ workflow_dispatch:
+ pull_request:
+ branches:
+ - main
+ paths:
+ - 'llvm/**/DirectX/**'
+ - '.github/workflows/hlsl*'
+ - clang/*HLSL*/**/*
+ - clang/**/*HLSL*
+ - llvm/**/Frontend/HLSL/**/*
+
+jobs:
+ macOS-Metal-Clang:
+ uses: ./.github/workflows/hlsl/hlsl-all.yaml
+ with:
+ OS: macOS
+ SKU: hlsl-metal
+ TestTarget: check-hlsl-clang-mtl
+ LLVM-branch: ${{ github.ref }}
diff --git a/.github/workflows/hlsl/hlsl-all.yaml b/.github/workflows/hlsl/hlsl-all.yaml
new file mode 100644
index 00000000000000..6195891a4ec0e7
--- /dev/null
+++ b/.github/workflows/hlsl/hlsl-all.yaml
@@ -0,0 +1,177 @@
+name: HLSL Test
+
+permissions:
+ contents: read
+ checks: write
+
+on:
+ workflow_dispatch:
+ inputs:
+ LLVM-Ref:
+ description: 'Test Suite Branch'
+ required: false
+ default: 'main'
+ type: string
+ LLVM-branch:
+ description: 'LLVM Branch'
+ required: false
+ default: 'main'
+ type: string
+ LLVM-fork:
+ description: 'LLVM fork'
+ required: false
+ default: 'llvm'
+ type: string
+ DXC-branch:
+ description: 'DXC Branch'
+ required: false
+ default: 'main'
+ type: string
+ BuildType:
+ description: 'Build Type'
+ required: false
+ default: 'Release'
+ type: choice
+ options:
+ - Release
+ - RelWithDebInfo
+ - Debug
+ TestTarget:
+ required: false
+ default: 'check-hlsl'
+ type: string
+ Test-Clang:
+ required: true
+ type: choice
+ options:
+ - On
+ - Off
+ OS:
+ required: true
+ type: choice
+ options:
+ - macOS
+ - windows
+ SKU:
+ required: true
+ type: choice
+ options:
+ - hlsl-metal
+ LLVM-ExtraCMakeArgs:
+ description: 'Extra CMake Args for LLVM'
+ required: false
+ default: ''
+ type: string
+ workflow_call:
+ inputs:
+ OffloadTest-branch:
+ description: 'Test Suite Branch'
+ required: false
+ default: 'main'
+ type: string
+ LLVM-branch:
+ description: 'LLVM Branch'
+ required: false
+ default: 'main'
+ type: string
+ LLVM-fork:
+ description: 'LLVM For'
+ required: false
+ default: 'llvm'
+ type: string
+ DXC-branch:
+ description: 'DXC Branch'
+ required: false
+ default: 'main'
+ type: string
+ OS:
+ required: true
+ type: string
+ SKU:
+ required: true
+ type: string
+ BuildType:
+ description: 'DXC Branch'
+ required: false
+ default: 'Release'
+ type: string
+ Test-Clang:
+ required: false
+ default: 'On'
+ type: string
+ TestTarget:
+ required: false
+ default: 'check-hlsl'
+ type: string
+ LLVM-ExtraCMakeArgs:
+ description: 'Extra CMake Args for LLVM'
+ required: false
+ default: ''
+ type: string
+
+jobs:
+ build:
+ runs-on: [self-hosted, "${{ inputs.SKU }}"]
+ steps:
+ - name: Checkout DXC
+ uses: actions/checkout@v4
+ with:
+ repository: Microsoft/DirectXShaderCompiler
+ ref: ${{ inputs.DXC-branch }}
+ path: DXC
+ fetch-depth: 1
+ submodules: true
+ - name: Checkout LLVM
+ uses: actions/checkout@v4
+ with:
+ repository: ${{ inputs.LLVM-fork }}/llvm-project
+ ref: ${{ inputs.LLVM-branch }}
+ path: llvm-project
+ fetch-depth: 1
+ - name: Checkout OffloadTest
+ uses: actions/checkout@v4
+ with:
+ repository: llvm-beanz/offload-test-suite
+ ref: ${{ inputs.OffloadTest-branch }}
+ path: OffloadTest
+ fetch-depth: 1
+ - name: Checkout Golden Images
+ uses: actions/checkout@v4
+ with:
+ repository: llvm-beanz/offload-golden-images
+ ref: main
+ path: golden-images
+ fetch-depth: 1
+ - name: Setup Windows
+ if: inputs.OS == 'windows'
+ uses: llvm/actions/setup-windows@main
+ with:
+ arch: amd64
+ - name: Build DXC
+ run: |
+ cd DXC
+ mkdir build
+ cd build
+ cmake -G Ninja -DCMAKE_BUILD_TYPE=${{ inputs.BuildType }} -C ${{ github.workspace }}/DXC/cmake/caches/PredefinedParams.cmake -C ${{ github.workspace }}/OffloadTest/cmake/caches/sccache.cmake -DHLSL_DISABLE_SOURCE_GENERATION=On ${{ github.workspace }}/DXC/
+ ninja
+ - name: Build LLVM
+ run: |
+ cd llvm-project
+ mkdir build
+ cd build
+ cmake -G Ninja ${{ inputs.LLVM-ExtraCMakeArgs }} -DDXIL_DIS=${{ github.workspace }}/DXC/build/bin/llvm-dis -DCMAKE_BUILD_TYPE=${{ inputs.BuildType }} -C ${{ github.workspace }}/llvm-project/clang/cmake/caches/HLSL.cmake -C ${{ github.workspace }}/OffloadTest/cmake/caches/sccache.cmake -DDXC_DIR=${{ github.workspace }}/DXC/build/bin -DLLVM_EXTERNAL_OFFLOADTEST_SOURCE_DIR=${{ github.workspace }}/OffloadTest -DLLVM_EXTERNAL_PROJECTS="OffloadTest" -DLLVM_LIT_ARGS="--xunit-xml-output=testresults.xunit.xml -v" -DOFFLOADTEST_TEST_CLANG=${{ inputs.Test-Clang }} -DGOLDENIMAGE_DIR=${{ github.workspace }}/golden-images ${{ github.workspace }}/llvm-project/llvm/
+ ninja hlsl-test-depends llvm-test-depends clang-test-depends
+ - name: Run HLSL Tests
+ run: |
+ cd llvm-project
+ cd build
+ ninja check-llvm
+ ninja check-clang
+ ninja check-hlsl-unit
+ ninja ${{ inputs.TestTarget }}
+ - name: Publish Test Results
+ uses: EnricoMi/publish-unit-test-result-action/macos@v2
+ if: always() && inputs.OS == 'macOS'
+ with:
+ comment_mode: off
+ files: llvm-project/build/**/testresults.xunit.xml
|
I will not push this way!
.github/workflows/hlsl-test-all.yaml .github/workflows/hlsl-test-all.yaml
.github/workflows/hlsl-test-all.yaml
Outdated
| OS: | ||
| required: true | ||
| type: choice | ||
| options: | ||
| - macOS | ||
| - windows |
There was a problem hiding this comment.
You should be able to replace inputs.OS with runner.os in all places and drop this parameter completely. The OS values are: Linux, Windows, and macOS.
.github/workflows/hlsl-test-all.yaml
Outdated
| - name: Checkout LLVM | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: ${{ inputs.LLVM-fork }}/llvm-project |
There was a problem hiding this comment.
What's the purpose of have an LLVM-fork input ?
There was a problem hiding this comment.
I'll also remove this. It is useful in the tests on the the test suite repo because it enables running on a change that isn't merged to LLVM, but isn't useful here.
.github/workflows/hlsl-test-all.yaml
Outdated
| path: golden-images | ||
| fetch-depth: 1 | ||
| - name: Setup Windows | ||
| if: runner.os == 'windows' |
There was a problem hiding this comment.
This needs to be capitalized: 'Windows'
| - name: Checkout OffloadTest | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: llvm-beanz/offload-test-suite |
There was a problem hiding this comment.
I'm assuming this is the repository for https://discourse.llvm.org/t/rfc-proposal-for-offload-execution-test-suite/83947?
| - name: Checkout Golden Images | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: llvm-beanz/offload-golden-images |
There was a problem hiding this comment.
How are these images generated? Is there a reason this needs to be a separate repository? Are they expected to stay completely static?
When working with something like this in the past I shipped images as a Github release, but I would had to regenerate things version to version of the software that I was trying to support. If things are expected to remain static, keeping them in a git repo seems reasonable enough to me.
There was a problem hiding this comment.
They are expected to mostly remain static, but also expected to be significant in size as we flesh out the test coverage. They're in a separate repository from the other tests because they are optional. Most tests do not rely on image comparison for correctness.
.github/workflows/hlsl-test-all.yaml
Outdated
| repository: llvm-beanz/offload-test-suite | ||
| ref: ${{ inputs.OffloadTest-branch }} | ||
| path: OffloadTest | ||
| fetch-depth: 1 |
There was a problem hiding this comment.
I believe this is the default and can probably be removed?
.github/workflows/hlsl-test-all.yaml
Outdated
| repository: llvm-beanz/offload-golden-images | ||
| ref: main | ||
| path: golden-images | ||
| fetch-depth: 1 |
.github/workflows/hlsl-test-all.yaml
Outdated
| ninja check-hlsl-unit | ||
| ninja ${{ inputs.TestTarget }} | ||
| - name: Publish Test Results | ||
| uses: EnricoMi/publish-unit-test-result-action/macos@v2 |
There was a problem hiding this comment.
This seems like a reasonable enough first approach for publishing test results to me. We need to figure out something like this for the main premerge pipeline and I'm not exactly sure what we want to do...
Good to know that this exists.
There was a problem hiding this comment.
It's a bit janky... I don't love it. I'm really annoyed that GitHub doesn't have first class support for test result publication. It is better than nothing 😢 .
.github/workflows/hlsl-test-all.yaml
Outdated
|
|
||
| jobs: | ||
| build: | ||
| runs-on: [self-hosted, "${{ inputs.SKU }}"] |
There was a problem hiding this comment.
I'm fairly certain you can remove the self-hosted here and just use ${{ inputs.SKU}}.
.github/workflows/hlsl-test-all.yaml
Outdated
| inputs: | ||
| OffloadTest-branch: | ||
| description: 'Test Suite Branch' | ||
| required: false | ||
| default: 'main' | ||
| type: string | ||
| LLVM-branch: | ||
| description: 'LLVM Branch' | ||
| required: false | ||
| default: 'main' | ||
| type: string | ||
| DXC-branch: | ||
| description: 'DXC Branch' | ||
| required: false | ||
| default: 'main' | ||
| type: string | ||
| SKU: | ||
| required: true | ||
| type: string | ||
| BuildType: | ||
| description: 'Build Type' | ||
| required: false | ||
| default: 'Release' | ||
| type: string | ||
| Test-Clang: | ||
| required: false | ||
| default: 'On' | ||
| type: string | ||
| TestTarget: | ||
| required: false | ||
| default: 'check-hlsl' | ||
| type: string | ||
| LLVM-ExtraCMakeArgs: | ||
| description: 'Extra CMake Args for LLVM' | ||
| required: false | ||
| default: '' | ||
| type: string |
There was a problem hiding this comment.
This is a lot of inputs do you really need all of these?
There was a problem hiding this comment.
I can strip down and simplify what is in the LLVM tree. They exist because they have all been used in the dispatch workflow on the test suite repo. It's useful for running one-off tests.
The workflow we use for LLVM pre-merge doesn't need that so I'll strip them down.
.github/workflows/hlsl-test-all.yaml
Outdated
| - name: Checkout LLVM | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: llvm/llvm-project |
There was a problem hiding this comment.
This should be $GITHUB_RESPOSITORY, which is the default, so you can actually just omit this.
.github/workflows/hlsl-test-all.yaml
Outdated
| runs-on: [self-hosted, "${{ inputs.SKU }}"] | ||
| steps: | ||
| - name: Checkout DXC | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
For this and all the actions we use, it should be pinned to a specific git hash and not use labels. See https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions
.github/workflows/hlsl-macos.yaml
Outdated
There was a problem hiding this comment.
I think the structure of these files is not going to scale well. The way you have it structured, you will need to add a new file for each OS and the files will be mostly the same. I think what you want is something more like this: https://github.com/llvm/llvm-project/blob/main/.github/workflows/release-binaries-all.yml#L73
Where you have the hlsl-test-all.yaml file with a job that has a matrix of all the supported operating systems, and then each matrix entry calls hlsl-test.yaml (which is what you now have in hlsl-test-all.yml) to run the actual tests.
There was a problem hiding this comment.
Yea, that makes sense. The structuring as it is was to produce status badges:
(see: https://github.com/llvm-beanz/offload-test-suite/?tab=readme-ov-file#current-status)
Those require unique workflows at the top level and can't provide matrix granularity.
Obviously this isn't a consideration here, so I can refactor this.
This adds a workflow for running HLSL tests on PRs that modify HLSL and DirectX code. The tests enabled here are the LLVM & Clang tests and the Offload execution tests: https://github.com/llvm-beanz/offload-test-suite/
This adds a workflow for running HLSL tests on PRs that modify HLSL and DirectX code.