[DRAFT/TEST] Validate PR #2398 fix on rocmlir-gen.cpp#2399
Draft
bogdan-petkovic wants to merge 9 commits into
Draft
[DRAFT/TEST] Validate PR #2398 fix on rocmlir-gen.cpp#2399bogdan-petkovic wants to merge 9 commits into
bogdan-petkovic wants to merge 9 commits into
Conversation
Signed-off-by: bogdan-petkovic <bpetkovi@amd.com>
Signed-off-by: bogdan-petkovic <bpetkovi@amd.com>
bogdan-petkovic
changed the base branch from
develop
to
users/bpetkovi/fix-cpp-static-check-missing-inc
rocmlir-pr-reviewer
Bot
added
the
modifies-ci-paths
|
bogdan-petkovic
changed the base branch from
users/bpetkovi/fix-cpp-static-check-missing-inc
to
develop
git restore-mtime walks the full commit graph to find the last commit that touched each path. With the external/llvm-project subtree this expands to ~100k+ commits and takes well over 15 minutes on the GHA runner. --first-parent restricts the walk to the mainline, which is sufficient for the rocMLIR working copy. --skip-missing tolerates index entries not touched by any first-parent commit so the step does not error out. Co-authored-by: Cursor <cursoragent@cursor.com>
Benign comment touch in:
- RockOps.td -> exercises TableGen-generated .inc regeneration plus
cascading rebuilds (Rock dialect headers consumed
transitively by many TUs).
- rocmlir-gen.cpp -> exercises clang-tidy on a TU that includes locally
generated .inc headers; recompiles a single .o.
Expectations on CI with the now-seeded cache:
- "Cache CMake build directory" -> exact-match HIT (no fallback).
- "Build TableGen-generated headers" -> ninja rebuilds only the
.td-derived .inc files and downstream objects (target <= 1-2 min,
vs. 7m39s on the cold seeding run).
Co-authored-by: Cursor <cursoragent@cursor.com>
git restore-mtime --first-parent could not locate ~128k files (almost
entirely external/llvm-project/**) because subtree updates enter the
history through the second parent of merge commits and are invisible
to --first-parent. --skip-missing left those files at the
actions/checkout default ("now"), so ninja saw the entire LLVM source
tree as newer than the cached .o files and rebuilt everything.
Fix: pre-touch every tracked file to a fixed old timestamp first, then
run git-restore-mtime --first-parent. Files on the mainline (rocMLIR
proper, including PR-touched .cpp/.td/.yml) get their accurate commit
time; files invisible to --first-parent stay at the old floor, which
is older than the cached build artifacts -> ninja keeps the cache.
Also pass --quiet to suppress the ~128k WARNING lines.
Co-authored-by: Cursor <cursoragent@cursor.com>
The previous run revealed that ninja was not rebuilding *anything*
even for files explicitly modified by the PR ("Build TableGen
headers" finished in ~2s for an .td-changing commit). Root cause:
On pull_request events actions/checkout@v4 checks out the test-merge
ref (refs/pull/N/merge). HEAD's first parent is the base branch and
its second parent is the PR HEAD, so the PR's commits do NOT lie on
HEAD's first-parent line. Combined with the fact that
git-restore-mtime uses `git whatchanged` (which skips merge commit
diffs without `-m`), this means every PR-modified file inherits the
last develop commit's timestamp that touched it -- always older than
the cached .o that was built from that develop commit. ninja keeps
the stale .o; clang-tidy parses the old .inc headers, masking real
issues introduced by .td changes.
Fix: after git-restore-mtime, run `git diff --name-only BASE...HEAD`
and touch every file in the result. `touch` (no -d) sets mtime to
"now", which is guaranteed newer than any cached build artifact, so
ninja rebuilds exactly the PR-modified set and the TableGen-derived
.inc headers used by clang-tidy.
The remaining staleness window for develop-side real merges that
touch C++ files is left as a known limitation; in practice the
develop CI re-seeds the cache after every merge so the window is
bounded to a single run.
Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2399 +/- ##
===========================================
+ Coverage 79.50% 82.09% +2.58%
===========================================
Files 100 119 +19
Lines 31016 42672 +11656
Branches 4819 7082 +2263
===========================================
+ Hits 24659 35028 +10369
- Misses 4245 5053 +808
- Partials 2112 2591 +479
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Verify that PR #2398 (build TableGen headers before clang-tidy) fixes
the missing
.h.incfailure class that PR #2310 hit. This branch isbased on the PR #2398 branch and touches
rocmlir-gen.cpp, the onlyC++ file PR #2310 modified, with a benign comment change so that
clang-tidy-diffruns on the file and is forced to parse the fullTU (Rock dialect headers + transitively included
.incfiles).Test Plan
Watch the
cpp-static-checksjob:file not found ... .h.inc-> fix has a gap; expand theTableGen target list in PR Fix cpp-static-checks: build TableGen headers before clang-tidy #2398.
DRAFT: do not merge.