Docker workflow for gap finding and gap filling - #82
Conversation
|
It could be a good idea to allow this workflow to work independently of where the coverage profile comes from. For example, right from baseline or from the uncovered lines of a PR. At the current stage, "incremental" can't work in this way and always expects an output folder. This PR #88 refactors it so we can explicitly specify the required CSV files. This new workflow could re-use an existing image if available. |
Just to update my last comment, this is now solved in main. |
Stage only the first N candidate tests at run time and bind-mount them into a one-shot container; baseline-only mode skips the candidate tests.
Describe baseline-only and full-pipeline modes for baseline-coverage-gap-fill.sh, including runtime candidate staging.
Avoid early exit 0 under set -e in bash -lc; wrap later pipeline steps in a conditional instead.
6bc2581 to
b75700c
Compare
Write target_lines_uncovered.csv with file/line columns and absolute paths matching line_coverage_uncovered.csv so downstream incremental analysis can consume WF2 output directly.
Replace directory-based --baseline-dir and --wf2-output-dir with --line-coverage-uncovered-csv and --llc-address-line-map-csv to match coverage incremental. Update test_coverage_amdgpu_workflow1.sh to drop the removed --baseline-output-dir flag.
Share image resolution, skip-if-exists build, --force-build, and cleanup logic between baseline-coverage-gap-fill.sh and pr-cov-gaps-detection.sh.
…ontract. Update README for explicit incremental CSV inputs, ensure-image.sh reuse semantics, and Workflow 1/2 Docker chaining examples.
Add gap-finding-baseline.sh (baseline-only), gap-finding-pr.sh (PR target-lines), and gap-filling-amdgpu.sh (candidate-test + incremental). Use baseline terminology throughout instead of suite.
Add gap-finding-baseline.sh, gap-finding-pr.sh, and gap-filling.sh. Gap filling requires explicit profile CSVs; baseline no longer runs inline.
Delete test_coverage*.sh and baseline-coverage-gap-fill / pr-cov-gaps-detection Docker runners; update lit-filters-amdgpu.sh refs.
Document new script names, Docker split, chaining examples, and PR vs local commit gap finding paths.
ambergorzynski
changed the title
Avoid host-side copy of irtests inputs; -n limits work in candidate-test. Add --stage-candidate-tests to restore temp-dir staging when needed.
Explain default irtests mount, -n limiting, and --stage-candidate-tests opt-in.
Wrap batch_reduce_using_coverage.py --n 1 against incremental/new_coverage.csv and candidate_tests/ under a gap-filling output directory.
Mount gap-fill output and run batch_reduce_using_coverage.py --n 1 in-container.
README covers the gap-reducing scripts and chaining after gap fill. batch_reduce now handles timeout-wrapped llc invocations, .ll inputs, and Docker-to-host path remapping from gap-filling output.
| 2. **Gap filling** — run a fuzz corpus against that list and report which tests cover the gaps; then reduce promising tests into minimal LIT cases. | ||
|
|
||
| See [here](#contributions) for a list of tests contributed to LLVM. | ||
| Gap finding comes in two flavours: |
There was a problem hiding this comment.
I think the table is a bit hard to understand because in both cases we run a filtered set of LIT tests. What changes substantially is what gaps we care.
Gaps finding can detect gaps in two settings: a) changed lines in a PR or b) in the full codebase (i.e., AMDGPU backend).
Sharing the output files at this stage looks too early, we haven't explained how to execute these two modes
Maybe a diagram would be better, showing that there are two paths that generate a gap report, which is later consumed by the gap filling process.
There was a problem hiding this comment.
Good point, I have added a diagram instead, let me know what you think.
| fi | ||
|
|
||
| if [[ "$fail_count" -gt 0 ]]; then | ||
| msg="WARNING: ${fail_count} LIT test(s) failed during the baseline run (e.g., failed a check, timed out, unresolved, or unexpectedly passed). |
There was a problem hiding this comment.
We should avoid duplicate code (in regard to the non-docker scripts). The non-docker version for example is not checking this (from what I understand). So their behaviour will diverge.
There was a problem hiding this comment.
Yes good point, I have refactored to avoid duplication by moving the actual python invocation into lib/, please review and let me know what you think. I think it is still nice to have a script to each run the docker and non-docker versions, but those should be relatively lightweight
| --llc-address-line-map-csv ./data/baseline-run/baseline/llc_address_line_map.csv \\ | ||
| --candidate-tests-dir /path/to/irtests/bitcode/amdgpu/all -n 100 -j "\$(nproc)" | ||
| $(basename "$0") --pr-id 203468 --output-dir ./data/pr-fill-100 \\ | ||
| --line-coverage-uncovered-csv ./data/gap-finding-pr-203468/commit_lines_report/target_lines_uncovered.csv \\ |
There was a problem hiding this comment.
Is it commit_lines_report?
There was a problem hiding this comment.
I don't understand this comment?
| # RUN: %coverage incremental --output-dir %t/incremental --sancov %sancov --line-coverage-uncovered-csv %t/profile/line_coverage_uncovered.csv --llc-address-line-map-csv %t/profile/llc_address_line_map.csv --candidate-tests-output-dir %t/candidate_tests | ||
|
|
||
| # RUN: test -f %t/incremental/new_coverage.csv | ||
| # RUN: %batch-reduce --csv %t/incremental/new_coverage.csv --candidate-tests %t/candidate_tests --output %t/reduced --n 1 --pipeline llvm_reduce_ir |
There was a problem hiding this comment.
Just for completness, I think this must be functionality present in fuzz-fill. That is when the filling is complete, and we have a "ready to use" test.
There was a problem hiding this comment.
It would be better to discuss ways to break this logic into smaller steps and design a CLI accordingly.
| # RUN: %coverage incremental --output-dir %t/incremental --sancov %sancov --line-coverage-uncovered-csv %t/profile/line_coverage_uncovered.csv --llc-address-line-map-csv %t/profile/llc_address_line_map.csv --candidate-tests-output-dir %t/candidate_tests | ||
|
|
||
| # RUN: test -f %t/incremental/new_coverage.csv | ||
| # RUN: %batch-reduce --csv %t/incremental/new_coverage.csv --candidate-tests %t/candidate_tests --output %t/reduced --n 1 --pipeline llvm_reduce_ir |
There was a problem hiding this comment.
Is the test checking that the reduction preserves the desired coverage?
| # PR_TARGET_CSV-DAG: file,line,text | ||
|
|
||
| # RUN: %coverage candidate-test --output-dir %t/candidate_tests_pr --llc %sancov-llc --candidate-tests-dir %S/fixtures/coverage-new-tests --n 1 | ||
| # RUN: %coverage incremental --output-dir %t/incremental_pr --sancov %sancov --line-coverage-uncovered-csv %t/pr_gaps/target_lines_uncovered.csv --llc-address-line-map-csv %t/profile/llc_address_line_map.csv --candidate-tests-output-dir %t/candidate_tests_pr |
There was a problem hiding this comment.
Is the output of each incremental step expected to be the same? If so, could we use the same prefixes or something like that to also encode that in the check?
|
Will this PR supersede #95? |
|
This is establishing the reduction also as a workflow, which is great. However, the reduction is not yet part of fuzz-fill (in its CLI) It may be better to have that done first and possibly in a new PR. Otherwise, it would be too difficult to review. Bringing it into fuzz-fill CLI could be a great way of reviewing that part of the project. |
No - they are different changes |
fuzz-fill does already have a I suggest that we work through this PR first, on the basis of |
This reverts commit ea44f29.
This reverts commit 5b5ba58.
Extract reusable coverage, gap-finding, gap-filling, and gap-reducing functions so local and Docker entrypoints can share one implementation. No entrypoints wired up yet.
Wire gap-finding-baseline, gap-finding-pr, gap-filling-amdgpu, and gap-reducing-amdgpu through shared lib helpers. Align PR script step order and lit-allow-failures with Docker.
| | `--line-coverage-uncovered-csv <path>` | Gap list CSV (required) | | ||
| | `--llc-address-line-map-csv <path>` | LLC map from same baseline (required) | | ||
| | `--candidate-tests-dir <path>` | Host corpus root, bind-mounted read-only (required) | | ||
| | `-n <N>`, `--n <N>` | First N candidate tests to run (required) | |
There was a problem hiding this comment.
Could we process all the tests by default and cap the number only if requested? In the current way, it is not possible to let the step process all the candidate tests. I think the goal is to use the candidate-tests we provide, and we have kept them at a reasonable size so they can be processed all in a short time. Otherwise, I think this may force the user to speculate about the number of tests to pick, and whether it was enough.
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" | ||
| OUTPUT_DIR="${OUTPUT_DIR:-${REPO_ROOT}/data/coverage_output/bb_coverage_amdgpu_$(date +%y%m%d)}" |
There was a problem hiding this comment.
The default value looks too verbose, seems more like a quick test default name.
2 participants
This PR adds Docker support for the full coverage gap-fill pipeline, aligns gap-list CSV formats across baseline and PR flows, and reorganizes scripts and docs around gap finding vs gap filling rather than workflow 1 vs workflow 2.
Gap finding produces an uncovered-lines list (baseline-wide or PR-specific).
Gap filling runs candidate-test → incremental against that list.
Gap reduction reduces a test that is flagged as useful in the incremental step.
Docker runners are split accordingly.