-
Notifications
You must be signed in to change notification settings - Fork 18k
[Github][RFC] Add workflow to diff codegen on llvm-test-suite #190010
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
Changes from 2 commits
f208b30
ba5f906
68707da
af377f0
56563cf
4465c79
fc12789
0cd6c26
387d32b
9a18f4a
6fccd7b
765dc04
aa01aa3
ad54fac
fc4e6af
4aec090
3505249
cc3e4c6
79560bb
9563694
e16151c
399653c
c6d774f
3c33b26
f7ba728
eeee3bc
cd8ffdf
4fb58ee
6ac80fd
0521211
9aa7930
a37a98e
b3e1e01
576de46
f0f7a3f
4b1dd8e
69d91de
10594a5
f55d071
7e1c645
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,110 @@ | ||||||||||||||||||||||||||
| # When /test-suite is commented on a PR, checks out the PR, builds clang and | ||||||||||||||||||||||||||
| # then the test-suite in several configurations. It then checks out the base of | ||||||||||||||||||||||||||
| # the PR, builds clang and the test-suite again, and then uploads the diff of | ||||||||||||||||||||||||||
| # the codegen. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| name: Diff test-suite codegen | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
boomanaiden154 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||
| issue_comment: | ||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||
| - created | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||
| test-suite: | ||||||||||||||||||||||||||
| name: Build and diff | ||||||||||||||||||||||||||
| runs-on: ubuntu-24.04 | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you look at trying to use the CI container for this workflow? It should be about the same, just have a (significantly) faster default toolchain and also enable the use of precompiled headers given we use clang instead of gcc.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in ad54fac. Kicked off a test run with it here: https://github.com/lukel97/llvm-project/actions/runs/23910646008
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow the build is almost twice as fast. The only issue is that it looks like the cross compile builds are failing because the version of CMake is too old. It looks like it's version 3.28 on the image but we need at least 3.29 since the toolchain files use CMAKE_LINKER_TYPE. The default github ubuntu runner has 3.31: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#tools. Are you able to update the CMake version in the image or should we try and install a newer version in this workflow?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the CMake version in this image for now in cc3e4c6
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could, but ubuntu 26.04 also comes out in a week or two, so I think we can just wait until then to upgrade.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the timetable from here it will take some time before the 26.04 images will be available:
|
||||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||||
| issues: write | ||||||||||||||||||||||||||
| if: >- | ||||||||||||||||||||||||||
| !startswith(github.event.comment.body, '<!--IGNORE-->') && | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming this is to avoid some interaction with existing automation? Can you add a comment?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I copied this from issue-release-workflow.yml which has the |
||||||||||||||||||||||||||
| github.event.issue.pull_request && contains(github.event.comment.body, '/test-suite') | ||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||
| - id: get-pr | ||||||||||||||||||||||||||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||
| const { data: pr } = await github.rest.pulls.get({ | ||||||||||||||||||||||||||
| owner: context.repo.owner, | ||||||||||||||||||||||||||
| repo: context.repo.repo, | ||||||||||||||||||||||||||
| pull_number: context.payload.issue.number | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| if (!pr.mergeable) | ||||||||||||||||||||||||||
| await github.rest.issues.createComment({ | ||||||||||||||||||||||||||
| owner: context.repo.owner, | ||||||||||||||||||||||||||
| repo: context.repo.repo, | ||||||||||||||||||||||||||
| body: "Can't diff PR, PR isn't mergeable" | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| return pr | ||||||||||||||||||||||||||
|
Comment on lines
+39
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you need this step, isn't all the PR information available via the context?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately we only get the issue object fields so it's missing PR-specific things like the head ref, merge commit, mergability status etc. E.g. if you check the documentation on the payload object, the FWIW I was also surprised it was this awkward to get the PR info out of the event |
||||||||||||||||||||||||||
| - if: ${{ !fromJSON(steps.get-pr.outputs.result).mergeable }} | ||||||||||||||||||||||||||
| run: exit 1 | ||||||||||||||||||||||||||
| - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||
| github.rest.reactions.createForIssueComment({ | ||||||||||||||||||||||||||
| owner: context.repo.owner, | ||||||||||||||||||||||||||
| repo: context.repo.repo, | ||||||||||||||||||||||||||
| comment_id: context.payload.comment.id, | ||||||||||||||||||||||||||
| content: '+1' | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
|
boomanaiden154 marked this conversation as resolved.
|
||||||||||||||||||||||||||
| ref: ${{ fromJSON(steps.get-pr.outputs.result).merge_commit_sha }} | ||||||||||||||||||||||||||
| repository: ${{ fromJSON(steps.get-pr.outputs.result).head.repo.full_name }} | ||||||||||||||||||||||||||
| fetch-depth: 2 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| - run: | | ||||||||||||||||||||||||||
| echo "HEAD_SHA=$(git rev-parse HEAD)" >> $GITHUB_ENV | ||||||||||||||||||||||||||
| echo "BASE_SHA=$(git rev-parse HEAD^)" >> $GITHUB_ENV | ||||||||||||||||||||||||||
| echo "$GITHUB_WORKSPACE/.github/workflows/test-suite" >> $GITHUB_PATH | ||||||||||||||||||||||||||
| - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| repository: llvm/llvm-test-suite | ||||||||||||||||||||||||||
| path: llvm-test-suite | ||||||||||||||||||||||||||
|
Comment on lines
+64
to
+68
Check noticeCode scanning / zizmor credential persistence through GitHub Actions artifacts Note test
credential persistence through GitHub Actions artifacts
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you comment on this please?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to set
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 6fccd7b |
||||||||||||||||||||||||||
| - name: Install system dependencies | ||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||
| sudo apt-get update | ||||||||||||||||||||||||||
| sudo apt-get install -y cmake ninja-build libc6-dev-{arm64,riscv64}-cross libgcc-14-dev-{arm64,riscv64}-cross libstdc++-14-dev-{arm64,riscv64}-cross | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider building a custom Docker image to avoid having to install these packages on every run. I also think it'd be preferable to use compiler-rt and libc++ over libgcc and libstdc++ (and eventually also LLVM libc).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docker image makes sense, do you want me to do that as a follow up? FWIW these packages are pretty light so far, it takes about 19 seconds to install: https://github.com/lukel97/llvm-project/actions/runs/23885053232/job/69646132003#step:9:1 Agreed it would be nice to eventually use compiler-rt/libc++. But there aren't any
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd build them in the same build using the runtimes build, but that can be also done in a follow up change. It's definitely going to take longer than 19 seconds since you need to run CMake and Ninja multiple times but I still think it's desirable since that way you can also measure the impact of runtime changes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A custom docker image for this might be a bit heavyweight. Although it wouldn't hurt if we use the CI container to take advantage of the faster toolchain there. |
||||||||||||||||||||||||||
| - name: Configure Clang | ||||||||||||||||||||||||||
| run: cmake -B build -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD='AArch64;X86;RISCV' -DLLVM_ENABLE_PROJECTS='clang;lld' -DLLVM_APPEND_VC_REV=OFF llvm -GNinja | ||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd consider moving these options to a cache file or use one of existing ones from https://github.com/llvm/llvm-project/tree/main/clang/cmake/caches.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to a dedicated cache file in fc12789 |
||||||||||||||||||||||||||
| - name: Build Clang @ head | ||||||||||||||||||||||||||
| run: ninja -C build | ||||||||||||||||||||||||||
| - name: Configure and build test-suite @ head | ||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||
| build-and-configure.sh rva23u64-O3-b riscv64.cmake | ||||||||||||||||||||||||||
| build-and-configure.sh armv9-a-O3-b aarch64.cmake | ||||||||||||||||||||||||||
| build-and-configure.sh x86_64-O3-b x86_64.cmake | ||||||||||||||||||||||||||
| working-directory: llvm-test-suite | ||||||||||||||||||||||||||
| - name: Build test-suite @ base | ||||||||||||||||||||||||||
| run: git checkout $BASE_SHA && ninja -C build | ||||||||||||||||||||||||||
| - name: Configure and build test-suite @ base | ||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||
| build-and-configure.sh rva23u64-O3-a riscv64.cmake | ||||||||||||||||||||||||||
| build-and-configure.sh armv9-a-O3-a aarch64.cmake | ||||||||||||||||||||||||||
| build-and-configure.sh x86_64-O3-a x86_64.cmake | ||||||||||||||||||||||||||
| working-directory: llvm-test-suite | ||||||||||||||||||||||||||
| - run: | | ||||||||||||||||||||||||||
| mkdir diffs | ||||||||||||||||||||||||||
| ./utils/tdiff.py -a build.rva23u64-O3-a -b build.rva23u64-O3-b -s all > diffs/rva23u64-O3.diff || true | ||||||||||||||||||||||||||
| ./utils/tdiff.py -a build.armv9-a-O3-a -b build.armv9-a-O3-b -s all > diffs/armv9-a-O3.diff || true | ||||||||||||||||||||||||||
| ./utils/tdiff.py -a build.x86_64-O3-a -b build.x86_64-O3-b -s all > diffs/x86_64-O3.diff || true | ||||||||||||||||||||||||||
|
lukel97 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||
| working-directory: llvm-test-suite | ||||||||||||||||||||||||||
| - uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f #v7.0.0 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| id: upload-diffs | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| name: diffs | ||||||||||||||||||||||||||
| path: llvm-test-suite/diffs | ||||||||||||||||||||||||||
| - uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f #v7.0.0 | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| name: results | ||||||||||||||||||||||||||
| path: llvm-test-suite/results*.json | ||||||||||||||||||||||||||
| - uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 | ||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||
| DIFF_URL: ${{ steps.upload-diffs.outputs.artifact-url }} | ||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||
| github.rest.issues.createComment({ | ||||||||||||||||||||||||||
| owner: context.repo.owner, | ||||||||||||||||||||||||||
| repo: context.repo.repo, | ||||||||||||||||||||||||||
| issue_number: context.issue.number, | ||||||||||||||||||||||||||
| body: `test-suite diff from ${process.env.BASE_SHA}...${process.env.HEAD_SHA}: ${process.env.DIFF_URL}` | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| set(CMAKE_SYSTEM_NAME Linux) | ||
| set(CMAKE_C_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang) | ||
| set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang++) | ||
| set(CMAKE_C_COMPILER_TARGET aarch64-linux-gnu) | ||
| set(CMAKE_CXX_COMPILER_TARGET aarch64-linux-gnu) | ||
| set(CMAKE_C_FLAGS_INIT "-march=armv9-a -save-temps=obj") | ||
| set(CMAKE_CXX_FLAGS_INIT "-march=armv9-a -save-temps=obj") | ||
| set(CMAKE_SYSTEM_PROCESSOR arm64) | ||
| set(CMAKE_LINKER_TYPE LLD) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| #!/bin/bash | ||
|
|
||
| set -eux | ||
|
|
||
| cmake -B build.$1 \ | ||
| --toolchain $GITHUB_WORKSPACE/.github/workflows/test-suite/$2 \ | ||
| -C cmake/caches/O3.cmake \ | ||
| -GNinja \ | ||
| -DTEST_SUITE_BENCHMARKING_ONLY=ON \ | ||
| -DTEST_SUITE_RUN_BENCHMARKS=OFF | ||
| ninja -C build.$1 | ||
| $GITHUB_WORKSPACE/build/bin/llvm-lit build.$1 -o results.$1.json |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| set(CMAKE_SYSTEM_NAME Linux) | ||
| set(CMAKE_C_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang) | ||
| set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang++) | ||
| set(CMAKE_C_COMPILER_TARGET riscv64-linux-gnu) | ||
| set(CMAKE_CXX_COMPILER_TARGET riscv64-linux-gnu) | ||
| set(CMAKE_C_FLAGS_INIT "-march=rva23u64 -save-temps=obj") | ||
| set(CMAKE_CXX_FLAGS_INIT "-march=rva23u64 -save-temps=obj") | ||
| set(CMAKE_SYSTEM_PROCESSOR riscv64) | ||
| set(CMAKE_LINKER_TYPE LLD) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| set(CMAKE_C_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang) | ||
| set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/build/bin/clang++) | ||
| set(CMAKE_C_FLAGS_INIT "-save-temps=obj") | ||
| set(CMAKE_CXX_FLAGS_INIT "-save-temps=obj") | ||
| set(CMAKE_LINKER_TYPE LLD) |
Uh oh!
There was an error while loading. Please reload this page.