Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
f208b30
[Github][RFC] Add workflow to diff codegen on llvm-test-suite
lukel97 Apr 1, 2026
ba5f906
Address review comments
lukel97 Apr 2, 2026
68707da
Fix permissions
lukel97 Apr 2, 2026
af377f0
Use scripts + cmake files from default branch of upstream repo
lukel97 Apr 2, 2026
56563cf
Fix typo
lukel97 Apr 2, 2026
4465c79
Rename a and b to head and base
lukel97 Apr 2, 2026
fc12789
Use cmake cache for configuring llvm
lukel97 Apr 2, 2026
0cd6c26
Clone llvm-project in a separate directory
lukel97 Apr 2, 2026
387d32b
Add lit to path
lukel97 Apr 2, 2026
9a18f4a
Split issue commenting into other workflow, drop pull_request: write …
lukel97 Apr 2, 2026
6fccd7b
persist-credentials: false
lukel97 Apr 2, 2026
765dc04
permissions: { contents: read }
lukel97 Apr 2, 2026
aa01aa3
Only accept comments that start with /test-suite
lukel97 Apr 2, 2026
ad54fac
Use CI image
lukel97 Apr 2, 2026
fc4e6af
Allow issue_comment in issue-write so we can use it from test-suite.yml
lukel97 Apr 2, 2026
4aec090
Use /bin/sh since CI image doesn't have bash
lukel97 Apr 2, 2026
3505249
Add missing "
lukel97 Apr 2, 2026
cc3e4c6
Update CMake
lukel97 Apr 6, 2026
79560bb
Install tcl, needed to build llvm-test-suite
lukel97 Apr 6, 2026
9563694
Fix typo
lukel97 Apr 6, 2026
e16151c
Fix comments file not being json
lukel97 Apr 7, 2026
399653c
Comment on run failure
lukel97 Apr 7, 2026
c6d774f
Pass PR number into issue-write.yml
lukel97 Apr 7, 2026
3c33b26
Use PR number not id
lukel97 Apr 7, 2026
f7ba728
Fix LLVM_APPEND_VC_REV typo
lukel97 Apr 7, 2026
eeee3bc
Pin ci container image
lukel97 Apr 7, 2026
cd8ffdf
Address zizmor template expansion warnings
lukel97 Apr 7, 2026
4fb58ee
Merge branch 'main' into test-suite-workflow
lukel97 Apr 13, 2026
6ac80fd
Merge branch 'main' of github.com:llvm/llvm-project into test-suite-w…
lukel97 Apr 16, 2026
0521211
Strip out changes from #192205
lukel97 Apr 16, 2026
9aa7930
Add TODO to remove cmake install step
lukel97 Apr 16, 2026
a37a98e
sccache llvm build
lukel97 Apr 16, 2026
b3e1e01
Merge branch 'main' into test-suite-workflow
lukel97 Apr 27, 2026
576de46
Merge branch 'main' into test-suite-workflow
lukel97 May 11, 2026
f0f7a3f
Merge branch 'main' of github.com:llvm/llvm-project into test-suite-w…
lukel97 May 12, 2026
4b1dd8e
Check actor is in llvm/llvm-committers team
lukel97 May 12, 2026
69d91de
Merge branch 'main' into test-suite-workflow
lukel97 May 18, 2026
10594a5
Merge branch 'main' into test-suite-workflow
lukel97 Jun 10, 2026
f55d071
Fix template injection warning
lukel97 Jun 10, 2026
7e1c645
Add TODO for reactions
lukel97 Jun 10, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/issue-write.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ on:
- "CI Checks"
- "Test Issue Write"
- "Check LLVM ABI annotations"
- "Diff test-suite codegen"
types:
- completed

Expand Down
156 changes: 156 additions & 0 deletions .github/workflows/test-suite.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
# When /test-suite is commented on a PR, checks out the PR, builds clang and
Comment thread
lukel97 marked this conversation as resolved.
# 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.

# TODO: React to the comment to let the user know the workflow has kicked
# off. We'll need to do this in a separate workflow like issue-write.yml with
# elevated permissions, since this workflow can't have write permissions on
# account for the arbitrary code execution.

name: Diff test-suite codegen

Comment thread
boomanaiden154 marked this conversation as resolved.
on:
issue_comment:
types:
- created

permissions:
contents: read

jobs:
test-suite:
name: Build test-suite and diff
runs-on: ubuntu-24.04

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the CMake version in this image for now in cc3e4c6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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:

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

container:
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Suggested change
- 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

@lukel97 lukel97 Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I forgot about that.

Doing it in a separate PR would be nice for testing anyways.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes it would be nice and it would highlight why we need the write permission. Thank you for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a TODO in 7e1c645

uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0
with:
script: |
await github.rest.teams.getMembershipForUserInOrg({
org: 'llvm',
team_slug: 'llvm-committers',
username: context.actor
})
- name: Get pull request
id: get-pr
uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.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
Comment on lines +39 to +48

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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?

@lukel97 lukel97 May 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 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

- name: Check pull request is mergeable
if: ${{ !fromJSON(steps.get-pr.outputs.result).mergeable }}
run: |
cat << EOF > comments
[{"body": "Unable to diff test-suite with PR, PR isn't mergeable"}]
EOF
exit 1
- name: Checkout pull request
uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
with:
Comment thread
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
path: llvm-project
persist-credentials: false
- name: Checkout llvm/llvm-test-suite
Comment on lines +56 to +64

Check warning

Code scanning / CodeQL

Checkout of untrusted code in a non-privileged context Medium test

Potential unsafe checkout of untrusted pull request on non-privileged workflow.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you comment on this please?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is fine given we're doing the permissions check for commit access above.

uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1
with:
repository: llvm/llvm-test-suite
path: llvm-test-suite
Comment on lines +64 to +68

Check notice

Code scanning / zizmor

credential persistence through GitHub Actions artifacts Note test

credential persistence through GitHub Actions artifacts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you comment on this please?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to set persist-credentials to false.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 6fccd7b

persist-credentials: false
- name: Setup environment variables
run: |
echo "HEAD_SHA=$(git rev-parse HEAD)" >> $GITHUB_ENV
echo "BASE_SHA=$(git rev-parse HEAD^)" >> $GITHUB_ENV
SCRIPTS_DIR="$GITHUB_WORKSPACE/llvm-project/.github/workflows/test-suite"
echo "SCRIPTS_DIR=$SCRIPTS_DIR" >> $GITHUB_ENV
echo "$SCRIPTS_DIR" >> $GITHUB_PATH
echo "$GITHUB_WORKSPACE/llvm-project/build/bin" >> $GITHUB_PATH
working-directory: llvm-project
- name: Install system dependencies
run: |
# Install newer version of CMake (llvm/ci-ubuntu-24.04 image has CMake 3.28)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a TODO (assign it to me if you want) to drop this once we're on 26.04?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 9aa7930

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need a newer cmake version here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need it specifically for CMAKE_LINKER_TYPE=LLD which is only available in 3.29. The cross compilation toolchain files need it so we can just use LLD instead of having to install several target specific linkers

# TODO(boomanaiden154): Remove once upgraded to Ubuntu 26.04
sudo apt-get update
sudo apt-get install -y wget
wget -O - https://apt.kitware.com/keys/kitware-archive-latest.asc 2>/dev/null | gpg --dearmor - | sudo tee /usr/share/keyrings/kitware-archive-keyring.gpg >/dev/null
echo 'deb [signed-by=/usr/share/keyrings/kitware-archive-keyring.gpg] https://apt.kitware.com/ubuntu/ noble main' | sudo tee /etc/apt/sources.list.d/kitware.list >/dev/null

sudo apt-get update
sudo apt-get install -y cmake tcl libc6-dev-arm64-cross libc6-dev-riscv64-cross libgcc-14-dev-arm64-cross libgcc-14-dev-riscv64-cross libstdc++-14-dev-arm64-cross libstdc++-14-dev-riscv64-cross
- name: Configure Clang
run: cmake -B build -C $SCRIPTS_DIR/llvm.cmake llvm -GNinja
working-directory: llvm-project
- name: Build Clang @ head
run: ninja -C build
working-directory: llvm-project
- name: Configure and build test-suite @ head
run: |
configure-and-build.sh rva23u64-O3-head $SCRIPTS_DIR/riscv64.cmake
configure-and-build.sh armv9-a-O3-head $SCRIPTS_DIR/aarch64.cmake
configure-and-build.sh x86_64-O3-head $SCRIPTS_DIR/x86_64.cmake
working-directory: llvm-test-suite
- name: Build Clang @ base
run: git checkout $BASE_SHA && ninja -C build
working-directory: llvm-project
- name: Configure and build test-suite @ base
run: |
configure-and-build.sh rva23u64-O3-base $SCRIPTS_DIR/riscv64.cmake
configure-and-build.sh armv9-a-O3-base $SCRIPTS_DIR/aarch64.cmake
configure-and-build.sh x86_64-O3-base $SCRIPTS_DIR/x86_64.cmake
working-directory: llvm-test-suite
- name: Compute diffs
run: |
mkdir diffs
./utils/tdiff.py -a build.rva23u64-O3-base -b build.rva23u64-O3-head -s all > diffs/rva23u64-O3.diff || true
./utils/tdiff.py -a build.armv9-a-O3-base -b build.armv9-a-O3-head -s all > diffs/armv9-a-O3.diff || true
./utils/tdiff.py -a build.x86_64-O3-base -b build.x86_64-O3-head -s all > diffs/x86_64-O3.diff || true
working-directory: llvm-test-suite
- name: Upload diffs
Comment thread Fixed
Comment thread Fixed
uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f #v7.0.0
id: upload-diffs
with:
name: diffs
path: llvm-test-suite/diffs
- name: Upload results
uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f #v7.0.0
with:
name: results
path: llvm-test-suite/results*.json
- name: Create comment
env:
ARTIFACT_URL: ${{ steps.upload-diffs.outputs.artifact-url }}
run: |
cat << EOF > comments
[{"body" : "test-suite diff from $BASE_SHA...$HEAD_SHA: $ARTIFACT_URL"}]
EOF
- name: Create comment on failure
if: failure()
env:
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
run: |
cat << EOF > comments
[{"body" : "Failed to get test-suite diff from $BASE_SHA...$HEAD_SHA: $RUN_URL"}]
EOF
- name: Save PR number
if: always()
env:
PR_NUMBER: ${{ fromJSON(steps.get-pr.outputs.result).number }}
run: echo $PR_NUMBER > pr_number
- name: Upload comment and PR number
uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0
if: always()
with:
name: workflow-args
path: |
comments
pr_number
9 changes: 9 additions & 0 deletions .github/workflows/test-suite/aarch64.cmake
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}/llvm-project/build/bin/clang)
set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/llvm-project/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)
12 changes: 12 additions & 0 deletions .github/workflows/test-suite/configure-and-build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/bin/sh

set -eux

cmake -B build.$1 \
--toolchain $2 \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should probably enable ccache here on the assumption that had and base are going to be pretty similar?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably not the llvm-test-suite build because the compiler will always be different anyway. But I've added sccache to the LLVM build in a37a98e

-C cmake/caches/O3.cmake \
-GNinja \
-DTEST_SUITE_BENCHMARKING_ONLY=ON \
-DTEST_SUITE_RUN_BENCHMARKS=OFF
ninja -C build.$1
llvm-lit build.$1 -o results.$1.json
6 changes: 6 additions & 0 deletions .github/workflows/test-suite/llvm.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
set(CMAKE_BUILD_TYPE Release CACHE STRING "")
set(LLVM_TARGETS_TO_BUILD "AArch64;RISCV;X86" CACHE STRING "")
set(LLVM_ENABLE_PROJECTS "clang;lld" CACHE STRING "")
set(LLVM_APPEND_VC_REV OFF CACHE BOOL "")
set(CMAKE_C_COMPILER_LAUNCHER sccache CACHE STRING "")
set(CMAKE_CXX_COMPILER_LAUNCHER sccache CACHE STRING "")
9 changes: 9 additions & 0 deletions .github/workflows/test-suite/riscv64.cmake
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}/llvm-project/build/bin/clang)
set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/llvm-project/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)
5 changes: 5 additions & 0 deletions .github/workflows/test-suite/x86_64.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
set(CMAKE_C_COMPILER $ENV{GITHUB_WORKSPACE}/llvm-project/build/bin/clang)
set(CMAKE_CXX_COMPILER $ENV{GITHUB_WORKSPACE}/llvm-project/build/bin/clang++)
set(CMAKE_C_FLAGS_INIT "-save-temps=obj")
set(CMAKE_CXX_FLAGS_INIT "-save-temps=obj")
set(CMAKE_LINKER_TYPE LLD)
Loading