[Github][RFC] Add workflow to diff codegen on llvm-test-suite#190010
Conversation
A common task when reviewing PRs in the LLVM subproject is checking out the PR locally, building it, running it on some benchmarks e.g. llvm-test-suite, and comparing the codegen against some known version. This is quite laborious though so this PR adds a GitHub workflow to automate the process. It's triggered by commenting "/test-suite" on a PR. The workflow will kick off, build clang with the head and base of the PR, build the benchmarks in llvm-test-suite for several configurations with each version of clang, compute the diff in the output assembly via the tdiff.py script, and then report back with the diffs in a comment. Here's an example where you can see the diff of a codegen change in the RISC-V backend on my fork: #5 (comment) At the moment it's very simple but could be fleshed out later. Currently it builds llvm-test-suite for handful of common configurations, some cross-compiled: - `-target aarch64-linux-gnu -march=armv9-a -O3` - `-target riscv64-linux-gnu -march=rva23u64 -O3` - `-target x86_64-linux-gnu -O3` We could eventually extend this to accept arbitrary targets and flags in the comment. It also just comments a link to download the codegen diffs, but could eventually also include some output from the ./utils/compare.py script about e.g. changes in code size or statistics. For now, those results are just uploaded as an artifact. In terms of worker resources, running it on the free GitHub hosted workers is good enough. Building Clang takes a while, over an hour, but building the test-suite only takes around 10 minutes. But we could stick it on something beefier if we wanted the feedback to be faster. This workflow requires the PR to be mergeable, as it wants to get the diff of the "mergeability" commit that GitHub generates for each PR. That way the diff is always between the latest version of the base branch and the PR, not the base branch at the time the PR was created.
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-github-workflow Author: Luke Lau (lukel97) ChangesA common task when reviewing PRs in the LLVM subproject is checking out the PR locally, building it, running it on some benchmarks e.g. llvm-test-suite, and comparing the codegen against some known version. The process is fairly laborious so this PR adds a GitHub workflow to automate it. It's triggered by commenting "/test-suite" on a PR. The workflow will kick off, build clang with the head and base of the PR, build the benchmarks in llvm-test-suite for several configurations with each version of clang, compute the diff in the output assembly via the tdiff.py script, and then report back with the diffs in a comment. Here's an example on my fork where you can see the diff of a codegen change in the RISC-V backend: lukel97#5 (comment) At the moment it's very simple but could be fleshed out later. Currently it builds llvm-test-suite for handful of common configurations, some cross-compiled:
We could eventually extend this to accept arbitrary targets and flags in the comment. It would also be nice to support LTO diffs in future but we will need to add some extra support in llvm-test-suite to extract the asm during the link step. It also just comments a link to download the codegen diffs, but could eventually also include some output from the ./utils/compare.py script about e.g. changes in code size or statistics. For now, those results are just uploaded as an artifact. In terms of worker resources, running it on the free GitHub hosted workers is good enough. Building Clang takes a while, over an hour, but building the test-suite only takes around 10 minutes. But we could stick it on something beefier if we wanted the feedback to be faster. This workflow requires the PR to be mergeable, as it wants to get the diff of the "mergeability" commit that GitHub generates for each PR. That way the diff is always between the latest version of the base branch and the PR, not the base branch at the time the PR was created. Full diff: https://github.com/llvm/llvm-project/pull/190010.diff 1 Files Affected:
diff --git a/.github/workflows/test-suite.ll b/.github/workflows/test-suite.ll
new file mode 100644
index 0000000000000..0d9c81fc74083
--- /dev/null
+++ b/.github/workflows/test-suite.ll
@@ -0,0 +1,148 @@
+# 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
+
+on:
+ issue_comment:
+ types:
+ - created
+
+jobs:
+ test-suite:
+ name: Build and diff
+ runs-on: ubuntu-24.04
+ permissions:
+ issues: write
+ if: >-
+ !startswith(github.event.comment.body, '<!--IGNORE-->') &&
+ 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
+ - 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:
+ 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
+ - uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
+ with:
+ repository: llvm/llvm-test-suite
+ path: llvm-test-suite
+ - 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
+ - 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
+ - name: Build Clang @ head
+ run: ninja -C build
+ - name: Configure and build test-suite @ head
+ run: |
+ cat << EOF > rva23u64.cmake
+ set(CMAKE_SYSTEM_NAME Linux)
+ set(CMAKE_C_COMPILER ${GITHUB_WORKSPACE}/build/bin/clang)
+ set(CMAKE_CXX_COMPILER ${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)
+ EOF
+ cat << EOF > armv9-a.cmake
+ set(CMAKE_SYSTEM_NAME Linux)
+ set(CMAKE_C_COMPILER ${GITHUB_WORKSPACE}/build/bin/clang)
+ set(CMAKE_CXX_COMPILER ${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)
+ EOF
+ cat << EOF > x86_64.cmake
+ set(CMAKE_C_COMPILER ${GITHUB_WORKSPACE}/build/bin/clang)
+ set(CMAKE_CXX_COMPILER ${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)
+ EOF
+ build_llvm_test_suite () {
+ cmake -B build.$1 -C cmake/caches/O3.cmake --toolchain $2 -DTEST_SUITE_BENCHMARKING_ONLY=ON -DTEST_SUITE_RUN_BENCHMARKS=OFF -GNinja
+ ninja -C build.$1
+ $GITHUB_WORKSPACE/build/bin/llvm-lit build.$1 -o results.$1.json
+ }
+ build_llvm_test_suite rva23u64-O3-b rva23u64.cmake
+ build_llvm_test_suite armv9-a-O3-b armv9-a.cmake
+ build_llvm_test_suite 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_llvm_test_suite () {
+ cmake -B build.$1 -C cmake/caches/O3.cmake --toolchain $2 -DTEST_SUITE_BENCHMARKING_ONLY=ON -DTEST_SUITE_RUN_BENCHMARKS=OFF -GNinja
+ ninja -C build.$1
+ $GITHUB_WORKSPACE/build/bin/llvm-lit build.$1 -o results.$1.json
+ }
+ build_llvm_test_suite rva23u64-O3-a rva23u64.cmake
+ build_llvm_test_suite armv9-a-O3-a armv9-a.cmake
+ build_llvm_test_suite 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
+ 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}`
+ })
|
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
Is it possible to refactor the CMake caches you're creating out into separate files?
* .ll -> .yml (doh) * Put cmake files in separate location * Add bash script for configuring and building llvm-test-suite
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Instead of using untrusted PR repo
| - name: Checkout llvm/llvm-test-suite | ||
| uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| repository: llvm/llvm-test-suite | ||
| path: llvm-test-suite |
Check notice
Code scanning / zizmor
credential persistence through GitHub Actions artifacts Note test
There was a problem hiding this comment.
Could you comment on this please?
There was a problem hiding this comment.
We need to set persist-credentials to false.
Done in ba5f906, but one thing to watch out for is that we need to do another checkout of the repository to make sure we use the scripts/cmake files in the upstream repository, not the PR which is untrusted: af377f0 The CodeQL workflow picked this up. But it still seems to complain about using the ./utils/tdiff.py scripts etc. in llvm-test-suite. I think that's a false positive though, since they're always from the main branch of llvm/llvm-test-suite and not e.g. the PR |
| 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 | ||
| - 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Moved to a dedicated cache file in fc12789
| - 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 *-cross packages for these on Ubuntu AFAICT. I guess we would need to build these as part of a runtimes build? But I haven't yet explored cross compiling those yet, and I'm not sure how much longer it would take the workflow to run
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
So we don't need to worry about any of the other clones clobbering anything
| permissions: | ||
| pull-requests: write | ||
| if: >- | ||
| !startswith(github.event.comment.body, '<!--IGNORE-->') && |
There was a problem hiding this comment.
I'm assuming this is to avoid some interaction with existing automation? Can you add a comment?
There was a problem hiding this comment.
I copied this from issue-release-workflow.yml which has the /cherry-pick command. I'm not actually sure what it's for. But I figured we should probably just restrict it to comments that start with a slash command anyway so removed it in aa01aa3
| uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| path: scripts | ||
| sparse-checkout: .github/workflows/test-suite |
There was a problem hiding this comment.
Why do we need to check this out separately instead of just pulling it from the main checkout?
There was a problem hiding this comment.
The checkout on line 53 can be from an arbitrary ref on an arbitrary fork. So a malicious actor could change the scripts in .github/workflows/test-suite/ to be whatever, something that steals env vars or something, which this workflow would then clone and execute later on.
The separate checkout here makes sure we always take the scripts from llvm/llvm-project, and on the main branch. So as long as no one publishes malicious scripts there we should be ok
There was a problem hiding this comment.
They already basically have full RCE privileges though through CMake?
If we want to improve the security here, we should be writing out the comment as a file and using the issue-write workflow so we don't need GITHUB_TOKEN to have permissions to write comments.
There was a problem hiding this comment.
Ah good point, sounds like issue-write.yml is exactly what I'm looking for. Changed to use it in 9a18f4a
There was a problem hiding this comment.
You'll need to precommit the update in issue-write.yml as the workflow definition is always taken from the repository's main branch.
I'll approve a PR doing that if you want to put one up.
There was a problem hiding this comment.
This workflow definition is taken from the default branch too, I've set my own forks default branch to this PR for testing. Do you still want me to precommit it?
| uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| repository: llvm/llvm-test-suite | ||
| path: llvm-test-suite |
There was a problem hiding this comment.
| - 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 |
There was a problem hiding this comment.
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.
| jobs: | ||
| test-suite: | ||
| name: Build and diff | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Done in ad54fac. Kicked off a test run with it here: https://github.com/lukel97/llvm-project/actions/runs/23910646008
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Updated the CMake version in this image for now in cc3e4c6
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looking at the timetable from here it will take some time before the 26.04 images will be available:
| Milestone | Optimistic | Most likely | Pessimistic |
|---|---|---|---|
Beta available (ubuntu-26.04) |
May 10 | May 20–28 | June 15 |
| GA on runners | Aug 15 | Sep 15 – Oct 1 | Nov 15 |
ubuntu-latest flip starts |
Sep 15 | Oct 15 – Nov 15 | Jan 2027 |
ubuntu-latest flip complete |
Nov 2026 | Jan – Feb 2027 | Apr 2027 |
| 22.04 deprecation begins | Oct 2026 | Dec 2026 – Feb 2027 | Apr 2027 |
|
Ping |
| 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 | ||
| }) | ||
| return pr |
There was a problem hiding this comment.
Why do you need this step, isn't all the PR information available via the context?
There was a problem hiding this comment.
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 issue.pull_request field only contains a handful of URLs: https://docs.github.com/en/webhooks/webhook-events-and-payloads#issue_comment
FWIW I was also surprised it was this awkward to get the PR info out of the event
This is split off from llvm#190010. We want to add a new workflow triggered whenever a comment is added to an issue (workflow_run.event == 'issue_comment'), that also writes an comment back via the issue_write workflow. However for issue_comment workflows, the head branch for the workflow won't be the head of the PR, but the default branch of the repository. So trying to fetch the PR based on the branch will fail. GitHub docs seem to recommend that the PR number is explicitly passed via an artifact in these cases: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#using-data-from-the-triggering-workflow This PR adds support for this so we can eventually leave comments from the test-suite.yml workflow
|
Hi, I really like this workflow idea, but it is a little bit of security risk. Could we limit this so that only people with commit access can trigger the workflow? Could you do something similar to this: https://github.com/llvm/llvm-project/pull/196769/changes#diff-25cf99555bf126ee82e57adcc61fc11b0d490fc2f12f10de105acd5d841fbe2cR49-R61 |
Also upgrade github-script to 9.0.0
I've added a check for |
tstellar
left a comment
tstellar
left a comment
There was a problem hiding this comment.
Thanks for making the changes. I have no more feedback, but you should wait for the other reviewers before committing.
This is split off from llvm#190010. We want to add a new workflow triggered whenever a comment is added to an issue (workflow_run.event == 'issue_comment'), that also writes an comment back via the issue_write workflow. However for issue_comment workflows, the head branch for the workflow won't be the head of the PR, but the default branch of the repository. So trying to fetch the PR based on the branch will fail. GitHub docs seem to recommend that the PR number is explicitly passed via an artifact in these cases: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#using-data-from-the-triggering-workflow This PR adds support for this so we can eventually leave comments from the test-suite.yml workflow
|
Ping for other reviewers |
This is split off from llvm#190010. We want to add a new workflow triggered whenever a comment is added to an issue (workflow_run.event == 'issue_comment'), that also writes an comment back via the issue_write workflow. However for issue_comment workflows, the head branch for the workflow won't be the head of the PR, but the default branch of the repository. So trying to fetch the PR based on the branch will fail. GitHub docs seem to recommend that the PR number is explicitly passed via an artifact in these cases: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#using-data-from-the-triggering-workflow This PR adds support for this so we can eventually leave comments from the test-suite.yml workflow
|
Ping |
There was a problem hiding this comment.
I think I can only comment and not help out that much. I'm not familiar with the utils/tdiff.py script. But I think the overall approach looks good, although very complex and probably tedious to maintain.
Are there easy steps that you could print to repeat a build locally?
Maybe it makes sense to have one comment that shows the process of how the build is going? I have something like the following in mind:
- Create comment (aka build log comment) as a reaction to the
/test-suitecomment (aka trigger comment) and say:
<!-- {github trigger comment id} -->
Thank you @${event.comment.user.login} for using the <a href="todo:link-to-documentation-here"><code>/test-suite</code></a> command <a href="${event.comment.htmlurl}">here</a>!
<sub>This very comment will be used to continuously log build state changes for your request. We decided to do this in addition to using Github's Check Runs below so you can inspect previous check runs better.</sub>- If the PR is not mergeable you can append to the comment:
Sorry, but this pull request is currently not mergeable.- You might also want to prevent retriggering
/test-suitefor the same git SHA to prevent attacks or duplicate works and then append to the build log comment:
The same build request exists for this pull request's SHA (<ENTER GIT SHA HERE>) <a href="<ENTER ACTIONS LOG URL>">here</a>.I hope you get the idea. For starters I've added comment about adding reactions to the initial trigger comment.
| jobs: | ||
| test-suite: | ||
| name: Build and diff | ||
| runs-on: ubuntu-24.04 |
There was a problem hiding this comment.
Looking at the timetable from here it will take some time before the 26.04 images will be available:
| Milestone | Optimistic | Most likely | Pessimistic |
|---|---|---|---|
Beta available (ubuntu-26.04) |
May 10 | May 20–28 | June 15 |
| GA on runners | Aug 15 | Sep 15 – Oct 1 | Nov 15 |
ubuntu-latest flip starts |
Sep 15 | Oct 15 – Nov 15 | Jan 2027 |
ubuntu-latest flip complete |
Nov 2026 | Jan – Feb 2027 | Apr 2027 |
| 22.04 deprecation begins | Oct 2026 | Dec 2026 – Feb 2027 | Apr 2027 |
| - name: Checkout pull request | ||
| uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| ref: ${{ fromJSON(steps.get-pr.outputs.result).merge_commit_sha }} | ||
| repository: ${{ fromJSON(steps.get-pr.outputs.result).head.repo.full_name }} | ||
| fetch-depth: 2 | ||
| path: llvm-project | ||
| persist-credentials: false | ||
| - name: Checkout llvm/llvm-test-suite |
There was a problem hiding this comment.
Could you comment on this please?
| - name: Checkout llvm/llvm-test-suite | ||
| uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 | ||
| with: | ||
| repository: llvm/llvm-test-suite | ||
| path: llvm-test-suite |
There was a problem hiding this comment.
Could you comment on this please?
| image: ghcr.io/llvm/ci-ubuntu-24.04:35e958d7f0b7 | ||
| if: github.event.issue.pull_request && startswith(github.event.comment.body, '/test-suite') | ||
| steps: | ||
| - name: Check permissions |
There was a problem hiding this comment.
This is nitpicking but I suggest to give the user some feedback by reacting to the comment with an EYES reaction. We've something similar here. And depending on the outcome you can remove the EYES and add a THUMBS_UP (see here).
| - name: Check permissions | |
| - name: Add reaction to trigger comment | |
| uses: actions/github-script@v9 | |
| with: | |
| script: | | |
| const mutation = `mutation($comment_id: ID!, $reaction: ReactionContent!) { | |
| addReaction(input: {subjectId: $comment_id, content: $reaction}) { | |
| reaction {content} | |
| subject {id} | |
| } | |
| }`; | |
| const variables = { | |
| comment_id: '${{ github.event.comment.node_id }}', | |
| reaction: 'EYES' | |
| } | |
| const result = await github.graphql(mutation, variables) | |
| console.log(result) | |
| - name: Check permissions |
There was a problem hiding this comment.
I actually originally had the action give a thumbs up react on the comment! But I removed it in 9a18f4a since it requires the issue: write permission which we've been going to some lengths to avoid. The TL;DR is that because we're executing arbitrary code we need the github token to have as minimal permissions as possible
I think we can probably do this if we create another workflow similar to issue-write.yml. Would you be happy if I do that as a follow up just so that this PR doesn't get too big?
There was a problem hiding this comment.
Ah, I forgot about that.
Doing it in a separate PR would be nice for testing anyways.
There was a problem hiding this comment.
Yes it would be nice and it would highlight why we need the write permission. Thank you for this.
boomanaiden154
left a comment
boomanaiden154
left a comment
There was a problem hiding this comment.
LGTM once kwk@ comment around reactions to the initial message are done.
|
@kwk thanks for the review, having a persistent comment that gets updated with the progress does sound like a good idea. But similar to reactions I think that would also require the
FWIW this PR should already post a comment if the PR isn't mergeable |
7 participants
A common task when reviewing PRs in the LLVM subproject is checking out the PR locally, building it, running it on some benchmarks e.g. llvm-test-suite, and comparing the codegen against some known version.
The process is fairly laborious so this PR adds a GitHub workflow to automate it. It's triggered by commenting "/test-suite" on a PR. The workflow will kick off, build clang with the head and base of the PR, build the benchmarks in llvm-test-suite for several configurations with each version of clang, compute the diff in the output assembly via the tdiff.py script, and then report back with the diffs in a comment.
Here's an example on my fork where you can see the diff of a codegen change in the RISC-V backend: lukel97#7 (comment)
At the moment it's very simple but could be fleshed out later. Currently it builds llvm-test-suite for handful of common configurations, some cross-compiled:
-target aarch64-linux-gnu -march=armv9-a -O3-target riscv64-linux-gnu -march=rva23u64 -O3-target x86_64-linux-gnu -O3We could eventually extend this to accept arbitrary targets and flags in the comment. It would also be nice to support LTO diffs in future but we will need to add some extra support in llvm-test-suite to extract the asm during the link step.
It also just comments a link to download the codegen diffs, but could eventually also include some output from the ./utils/compare.py script about e.g. changes in code size or statistics. For now, those results are just uploaded as an artifact.
In terms of worker resources, running it on the free GitHub hosted workers is good enough. Building Clang takes a while, over an hour, but building the test-suite only takes around 10 minutes. But we could stick it on something beefier if we wanted the feedback to be faster.
This workflow requires the PR to be mergeable, as it wants to get the diff of the "mergeability" commit that GitHub generates for each PR. That way the diff is always between the latest version of the base branch and the PR, not the base branch at the time the PR was created.