Repository navigation
orc pgo/02 skeleton counter layout - #725
makslevental wants to merge 5 commits into
Conversation
Add projects/orc-pgo: a static OrcPgo library linked against the static LLVM component libraries (never libLLVM), GoogleTest unit tests, and scripts/cpp_coverage.sh, which enforces 100% line/function coverage via the repo-root scripts/check_coverage.py. CounterLayout assigns positional counters (one per function entry plus one per successor/arm of conditional branches, multi-successor switches and i1 selects), so computing it on two copies of the same IR yields identical indices. CI runs the coverage gate on macOS arm64 and Linux x86_64/aarch64, using either the released mlir-wheel or, when called from build_llvm.yml, the wheel built in that run.
makslevental
left a comment
There was a problem hiding this comment.
Automated review (code, tests, comments, silent-failure passes). Inline comments below; summary:
Important
- The 100% gate can pass vacuously for
lib/files no test links (static archive members that aren't pulled in emit no coverage mapping, andcheck_coverage.pynever checks that every file under--sourcesappears in the report). CounterLayout::computemis-numbers a lazily loaded module (unmaterialized bodies count as definitions with zero sites).SameIRSameLayoutdoesn't test the property the design depends on (no bitcode round-trip, sites never compared).
Suggestions: Linux tool defaults in cpp_coverage.sh, push path filter, fixture shape (multi-site functions, skipped functions between eligible ones), comments describing not-yet-existing pieces, unused TestSupport helpers.
Outside this diff (scripts/check_coverage.py, shared, moved in #724):
- An unterminated
LCOV_EXCL_STARTsilently excludes the rest of the file (should fail at EOF). llvm-cov exportstderr is discarded on success, so "N functions have mismatched data" warnings (which drop those functions from the denominator) go unseen.- Output is still labelled "eudsl-llvmpy C++ coverage" when run for orc-pgo; a
--labelarg would fix it.
Verified fine: artifact names vs build_llvm.yml uploads, call-build-orc-pgo permissions, LLVM_DEFINITIONS handling, static (not dylib) LLVM linking, coverage-tool/compiler version matching in CI, bash 3.2 behavior of the SOURCES loop.
yes fix all of these |
… and docs check_coverage.py (shared with eudsl-llvmpy): - fail when a C/C++ source under --sources has no coverage record (e.g. a static library member no test links in) - fail on an LCOV_EXCL_START without a matching LCOV_EXCL_STOP - surface llvm-cov export warnings; fail on mismatched profile data - add --label instead of hardcoding eudsl-llvmpy cpp_coverage.sh: derive llvm-cov/llvm-profdata from $CC on Linux, fail loudly on missing tools, zero tests, no .profraw or no coverage objects; drop references to directories that don't exist yet. CounterLayout: assert the module is materialized; document the site and skip rules, the per-arm switch counting, why invoke/indirectbr/callbr are not profiled, and the identical-IR precondition. Tests now cover a bitcode round-trip with full site comparison, multiple sites in layout order, skipped functions between eligible ones, unprofiled terminators, duplicate-destination switches and cross-module lookup. Also pin the googletest download hash, drop TestSupport helpers that are not used yet, and add the workflow file to the push path filter.
All three are fixed in 000468d:
The checker also fails when a source file under |
Sites are counted from the condition that picks the successor, before the instruction executes; these terminators have no such condition, so the rationale is not about edge splitting.
makslevental
left a comment
There was a problem hiding this comment.
Test-focused review using mutation testing: 41 single-change mutants of lib/CounterLayout.cpp, each rebuilt and run in an isolated worktree.
No cheating. Expected indices are hand-written constants, not recomputed by the code under test. Each counting rule has a test that fails only when that rule is broken. The fixtures really contain what the tests claim:
- the invoke, indirectbr and callbr in
UnprofiledTerminators; - layout order that differs from RPO in
SitesAreContiguousInLayoutOrder; - a scalar i1 condition in
vecops.
No asserting loop can run zero times unnoticed. The bitcode round-trip test catches a mutant that the same-text test can't: sites ordered by memory address.
Results: 35 mutants killed, 1 equivalent (isDeclarationForLinker), 5 survived. The survivors are fixture gaps, covered by the first three inline comments:
- switch guard
> 2; isInternalNameascontains/"__orc"/"__";- linkonce functions skipped.
The other inline comments are about robustness and naming.
Kill the five surviving mutants: add a two-successor switch, near-miss internal names, and linkonce_odr/internal/weak/__-prefixed definitions among the skipped functions. Also guard lookup() and getFunction() before dereferencing, assert the unprofiled fixture's terminator kinds, wrap expectSameLayout in ASSERT_NO_FATAL_FAILURE, and rename AssignsDeterministicPositions to AssignsExpectedPositions.
|
Superseded by the LLVM-PGO-reuse redesign (tracking: #726). |
Second PR of the
orc-pgostack. It adds the project skeleton, the coverage gate and CI, plus the first library piece,CounterLayout.Project (
projects/orc-pgo)OrcPgois a static library. It links the static LLVM component libraries fromllvm_map_components_to_libnames, neverlibLLVM.dylib, so every process has exactly one copy of LLVM. On macOS,otool -Lon the test binary shows onlylibSystemandlibc++.FetchContent.unittests/TestSupporthas helpers for parsing IR, getting a hostTargetMachineand JITting a module, which later PRs use.CounterLayoutbr, each multi-successorswitchand eachi1select. Indices follow module and instruction order.available_externallyfunctions and__orc_pgo*internals are skipped.Coverage gate (
scripts/cpp_coverage.sh)-fprofile-instr-generate -fcoverage-mapping, runctest, merge the profiles, then require 100% line and 100% function coverage overinclude/,lib/andtools/using the repo-rootscripts/check_coverage.py.llvm-cov/llvm-profdatamust match the clang that produced the profiles, because raw.profrawformats are version-locked. On macOS that means Apple clang with thexcruntools; on Linux, setup_base's llvm-18.CI
build_test_orc_pgo.ymlruns the coverage gate on ubuntu-22.04, ubuntu-22.04-arm and macos-14, usingsetup_baselike the other workflows.build_test_release_eudsl.ymldoes:pip downloads the releasedmlir-wheelnormally, or downloads the wheel artifact from the caller's run underworkflow_call.build_llvm.ymlgets acall-build-orc-pgojob, so distro changes are tested against orc-pgo with the wheel built in that run.Also adds
/.claude/to.gitignore.