Skip to content

CI: run the Cranelift verifier. - #14207

Open
cfallin wants to merge 2 commits into
bytecodealliance:mainfrom
cfallin:verification-in-ci
Open

CI: run the Cranelift verifier.#14207
cfallin wants to merge 2 commits into
bytecodealliance:mainfrom
cfallin:verification-in-ci

Conversation

@cfallin

@cfallin cfallin commented Aug 25, 2026

Copy link
Copy Markdown
Member

This PR adds a GitHub workflow that runs on pushes to main and on PRs. It restores the verifier's SMT query cache from the shared actions cache, runs 'verify.sh rebuild-cache' on top of it, and (on main) republishes the rebuilt cache as the isle-veri-cache.tar.gz asset on the rolling dev release plus a run artifact for use by local developers.

It also adds a script, cranelift/isle/veri/setup/download-cache.sh, so a local user can download the current CI cache from the dev release and verify incrementally on top of it.

Note that the cache needs a unique key per run on main, because a cache entry is immutable once
created. Fortunately we can select the latest key by prefix.

This PR adds a GitHub workflow that runs on pushes to main and on
PRs. It restores the verifier's SMT query cache from the
shared actions cache, runs 'verify.sh rebuild-cache' on top of it, and
(on main) republishes the rebuilt cache as the isle-veri-cache.tar.gz
asset on the rolling dev release plus a run artifact for use by
local developers.

It also adds a script, cranelift/isle/veri/setup/download-cache.sh, so a
local user can download the current CI cache from the dev release and
verify incrementally on top of it.
@cfallin
cfallin requested review from a team as code owners August 25, 2026 19:14
@cfallin
cfallin requested review from alexcrichton and removed request for a team August 25, 2026 19:14
@cfallin

cfallin commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

cc @avanhatt @mmcloughlin

Let's see how long the run takes on the branch...

@cfallin

cfallin commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Hmm, it seems that new workflows don't run until merge to main -- do I need to merge PRs as part of a debug loop?! That's awfully silly... (good security reasons for this I suppose but still)

@avanhatt

Copy link
Copy Markdown
Member

@cfallin it seems like you can manually trigger without merging to main https://docs.github.com/en/actions/how-tos/manage-workflow-runs/approve-runs-from-forks

@cfallin

cfallin commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Unfortunately it seems there is no "Approve workflows to run" option, at least for me. I wonder if the set of workflows is somehow fixed based on what is in main so an entirely new workflow isn't picked up... @alexcrichton let us know if there's a better way to test, here!

@alexcrichton alexcrichton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it works I'd recommend moving this to main.yml because otherwise we need to reconfigure repository settings to actually gate on this check where if it's in main.yml we can just manage it via the text configuration there.

Comment thread .github/workflows/isle-veri.yml Outdated
Comment thread .github/workflows/isle-veri.yml Outdated
@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language labels Aug 25, 2026
@github-actions

Copy link
Copy Markdown

Subscribe to Label Action

cc @cfallin, @fitzgen

Details This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@cfallin
cfallin force-pushed the verification-in-ci branch 2 times, most recently from 1533989 to 9e2f162 Compare August 25, 2026 23:19
- Move job into `main.yml` workflow.
- Add branch spec to PR section.
@cfallin
cfallin force-pushed the verification-in-ci branch from 9e2f162 to 7b3702c Compare August 25, 2026 23:20
@cfallin

cfallin commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

Cool, the job finishes in 3h58m on a GitHub runner. That's definitely longer than I would have hoped for, but as long as the cache isn't dropped or mass-invalidated by some change it should be OK? (We should merge when the merge queue is otherwise quiet of course...)

@avanhatt

Copy link
Copy Markdown
Member

Woo! 🎉

A few things:

  1. It looks like for aarch64, 54 instantiations result in unknown for this timeout/these runners. We may be okay with that, but at that point, we may also want to think about running the fast config that skips known-slow/more likely to be unknown queries? Would also reduce the runtime.
  2. Did we also want to verify the covered midend rules in this PR? Looks like from the logs this did a full aarch64 and the sanity-check x64 expansions, but not opt.

Comment on lines +1408 to +1414
- name: Install SMT solvers (cvc5, z3)
if: steps.smt-solvers.outputs.cache-hit != 'true'
run: |
solver_dir="$HOME/.cache/smt-solvers"
mkdir -p "${solver_dir}/bin"
./cranelift/isle/veri/setup/install-cvc5.sh -i "${solver_dir}"
./cranelift/isle/veri/setup/install-z3.sh -b "${solver_dir}/bin"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In CI this step took 3 seconds, so perhaps overkill to cache this step? Are you worried about flaky installation though?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, no, I was just worried it would take longer (and didn't have numbers until actually creating the PR). I can remove the caching step here.

Comment on lines +1451 to +1464
- name: Update dev release
if: github.ref_name == 'main'
env:
GH_TOKEN: ${{ github.token }}
run: |
if gh release view dev >/dev/null 2>&1; then
gh release upload dev isle-veri-cache.tar.gz --clobber
else
# Can happen on the very first run, before publish-artifacts
# has created the dev release yet. The tarball is still
# available as a run artifact, and the next run will pick up
# the rebuilt cache from the actions cache anyway.
echo "::warning::The dev release does not exist yet; skipping the release upload."
fi

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should happen through the publish-artifacts.yml workflow which is already responsible for uploading artifacts. For example all artifacts from the commit are downloaded here, shuffled around here, and uploaded here.

There's custom handling of the gh-pages artifact and you can shuffle aruond isle-veri-cache.tar.gz in that workflow.

Comment on lines +1437 to +1439
# Only runs landing on `main` write to the shared cache; PR and
# release-branch runs restore it read-only.
if: github.ref_name == 'main'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be removed and the cache should always be saved, and that way PRs can incrementally build up their own cache. PR caches aren't read by main and all workflow runs have a "branch base" where PRs are a different base than the main branch.

Comment on lines +1422 to +1431
- name: Restore verifier SMT query cache
uses: actions/cache/restore@v5
with:
path: cranelift/isle/veri/cache
# Cache entries are immutable: each run saves under a unique key and
# restores the most recent entry by prefix. Bump `v1` if the cache
# format changes.
key: isle-veri-cache-v1-${{ github.run_id }}
restore-keys: |
isle-veri-cache-v1-

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this use actions/cache to auto restore/save? I commented below but I don't believe there's any need to conditionally save.

Additionally, instead of using github.run_id could the key be a hash of input files? For example maybe the veri source code and aarch64 ISLE? That way when nothing is changing we're not making more caches.

Also, does the cvc5/z3 version need to be factored into this cache key?

@alexcrichton

Copy link
Copy Markdown
Member

Oh, also, as-is this is going to run on every PR which while probably fine we perhaps want to tweak a bit to only run conditionally like we do for most other jobs. If you copy this configuration it'll only run with prtest:full or on the merge queue and if you copy this configuration there will be custom logic of when to trigger on PRs (found here.

do I need to merge PRs as part of a debug loop?! That's awfully silly...

I'll caution that this'll probably need an iteration or two of landing on main to fiddle with the logic around caching and the dev release and such. I agree it's unfortunate, but it's all we've got.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift Issues related to the Cranelift code generator isle Related to the ISLE domain-specific language

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants