[AMDGPU] Add cross-platform min_blocks_per_cu occupancy hint #2091
Workflow file for this run
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
| name: Check feature factorization | |
| on: | |
| workflow_dispatch: | |
| pull_request: | |
| types: | |
| - opened | |
| - reopened | |
| - synchronize | |
| concurrency: | |
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} | |
| cancel-in-progress: true | |
| jobs: | |
| check-feature-factorization: | |
| name: Check feature factorization | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| - name: Wait using dead-reckoning for builds to finish, before running, to save AI cost, if someone pushes many commits in a row | |
| run: sleep 1800 | |
| - name: Collect diffs and file lists | |
| id: changed | |
| env: | |
| BASE_REF: ${{ github.base_ref }} | |
| run: | | |
| MERGE_BASE=$(git merge-base "origin/$BASE_REF" HEAD) | |
| # Diff restricted to MODIFIED (not added/deleted/renamed) source files. Newly added files | |
| # are out of scope for this check — we only care about feature-specific code being grafted | |
| # onto pre-existing files. | |
| git diff --diff-filter=M "$MERGE_BASE...HEAD" \ | |
| -- '*.py' '*.cpp' '*.h' '*.hpp' '*.c' '*.cc' '*.cu' \ | |
| ':!tests/' ':!benchmarks/' ':!docs/' ':!scripts/' ':!misc/' \ | |
| > /tmp/source_diff.patch | |
| # List of source files MODIFIED in this PR (the universe being examined). | |
| git diff --name-only --diff-filter=M "$MERGE_BASE...HEAD" \ | |
| -- '*.py' '*.cpp' '*.h' '*.hpp' '*.c' '*.cc' '*.cu' \ | |
| ':!tests/' ':!benchmarks/' ':!docs/' ':!scripts/' ':!misc/' \ | |
| > /tmp/modified_source_files.txt | |
| if [ ! -s /tmp/source_diff.patch ]; then | |
| echo "skip=true" >> "$GITHUB_OUTPUT" | |
| echo "No modifications to existing source files." | |
| else | |
| echo "skip=false" >> "$GITHUB_OUTPUT" | |
| echo "Source diff: $(wc -l < /tmp/source_diff.patch) lines" | |
| echo "Modified source files: $(wc -l < /tmp/modified_source_files.txt)" | |
| fi | |
| - name: Install Cursor CLI | |
| if: steps.changed.outputs.skip != 'true' | |
| run: | | |
| curl https://cursor.com/install -fsS | bash | |
| echo "$HOME/.cursor/bin" >> $GITHUB_PATH | |
| - name: Check feature factorization with Cursor agent | |
| if: steps.changed.outputs.skip != 'true' | |
| env: | |
| CURSOR_API_KEY: ${{ secrets.CURSOR_KEY_HUGH }} | |
| run: | | |
| RESULT=$(agent -p "$(cat <<'PROMPT' | |
| You are reviewing a PR for one specific factorization smell: *feature-specific code being | |
| piled into a heavily-tracked core file* when it could instead live in its own | |
| feature-specific file (new or existing). The concern is NOT that the new code is in the | |
| "wrong" place semantically — it is usually topically related to the host file. The concern | |
| is that the host file is already a hot, central, frequently-edited file, and adding more | |
| self-contained feature code to it makes review, merge conflicts, and future churn worse. | |
| The fix is almost always: extract the feature-specific block into its own module/file. | |
| Inputs: | |
| - /tmp/source_diff.patch — unified diff of NON-TEST source files MODIFIED (not added, | |
| deleted, or renamed) in this PR (.py, .cpp, .h, .hpp, .c, .cc, .cu, excluding tests/, | |
| benchmarks/, docs/, scripts/, misc/) | |
| - /tmp/modified_source_files.txt — list of those modified source files | |
| You may inspect any file in the repo and run shell commands to gather signal. Useful ones: | |
| - git log --oneline -- <path> | wc -l # total commit count for the file | |
| - git log --since=1.year.ago --oneline -- <path> | wc -l # recent churn | |
| - git shortlog -sn -- <path> | wc -l # distinct authors | |
| - wc -l <path> # current size | |
| Repo layout: | |
| - Python source: python/quadrants/ | |
| - C++ source: quadrants/ (subdirs: codegen, ir, runtime, transforms, rhi, python, etc.) | |
| The repo factorizes per-backend / per-pass / per-feature (separate files per RHI backend | |
| under quadrants/rhi/, separate files per IR transform under quadrants/transforms/, | |
| separate Python modules per concept under python/quadrants/lang/). | |
| Procedure (apply to each modified existing file): | |
| 1. Is the host file *heavily tracked / core*? Treat it as such if any of these hold: | |
| - High lifetime commit count (e.g. >= 100 commits). | |
| - High recent churn (e.g. >= 20 commits in the last year). | |
| - Many distinct authors (e.g. >= 8). | |
| - It is large (e.g. >= 800 lines) AND clearly central (frequently imported, sits in a | |
| core subdir like codegen/, ir/, runtime/, transforms/, lang/, lang/ast/). | |
| If NONE of these hold, do not flag anything in this file — the host is not hot enough | |
| to warrant carving things out. | |
| 2. If the host IS hot, examine the substantial added code. In scope: | |
| - New top-level functions or classes. | |
| - Large new self-contained blocks. | |
| - New methods added to an *existing* class. This is one of the most common shapes of | |
| this smell: a feature accretes methods on a central class living in a hot file. | |
| Suggested fixes are typically: extract the feature logic into free functions in a | |
| new feature module that the class delegates to (one-line forwarding methods are | |
| fine), or split the class via a mixin / partial / sibling helper class living in | |
| its own file. | |
| 3. Flag a block only if it is plausibly extractable: it has a clear feature/subsystem | |
| name, it is mostly self-contained (limited entanglement with the host file's existing | |
| internals beyond a small interface), and a reasonable file path can be named for it. | |
| 4. For each violation, suggest a better location: an *existing* feature-specific file or | |
| a plausible new file path that matches the repo's naming conventions. Briefly note the | |
| host's hotness numbers (commits / authors / size) so the reviewer can sanity-check. | |
| What NOT to flag: | |
| - Anything in a file that is not heavily-tracked / core (most files). | |
| - Small tweaks, bug fixes, or extensions of functions that already live in the host file. | |
| - Code that is tightly coupled to the host file's internals and would not cleanly extract. | |
| - Refactors / renames / moves that just relocate existing code. | |
| - Newly added files (out of scope for this check). | |
| - Trivial changes (imports, type hints, comments, docstrings, logging). | |
| - Cases where there is no clear better home and inventing a new file would be overkill. | |
| Be conservative. The bar is: "this PR is making a hot file hotter for no good reason, and | |
| the new block obviously wants to be its own file." Borderline cases are FINE. | |
| Do NOT modify any files. | |
| Stop after finding 5 violations. | |
| If there are NO violations, your final output must start with the word PASS. | |
| If there ARE violations, your final output must start with the word FAIL, followed by a | |
| list of violations (up to 5) in the format: | |
| <host_filepath> [commits=N, authors=M, lines=L]:<symbol_or_block>: <why extractable> -> suggested: <filepath> | |
| PROMPT | |
| )" --model claude-4.6-opus-high-thinking --mode ask --output-format text --trust) | |
| echo "$RESULT" | |
| if echo "$RESULT" | grep -qE "^FAIL([[:space:]]|$)"; then | |
| exit 1 | |
| fi |