[CI] allow dirty workspace for format.sh and introduce loop carry thread sync unit test
#1153
Conversation
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
|
👋 Hi! Thank you for contributing to the TileLang project. Please remember to run We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀 |
WalkthroughThe PR modifies Changes
Sequence DiagramsequenceDiagram
participant User
participant format.sh
participant MergeBase
participant WorkingTree
participant PreCommit
User->>format.sh: Run (no arguments)
activate format.sh
format.sh->>format.sh: Set ONLY_CHANGED=1
format.sh->>MergeBase: Get merge-base files
format.sh->>WorkingTree: Get working-tree files
format.sh->>format.sh: Compute CHANGED_FILES
alt CHANGED_FILES non-empty
format.sh->>format.sh: Print file list
format.sh->>format.sh: Convert to space-separated string
format.sh->>PreCommit: Run with --files <list>
PreCommit->>PreCommit: Format/check files
else CHANGED_FILES empty
format.sh->>format.sh: Print "no changes" info
end
deactivate format.sh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
testing/python/transform/test_tilelang_transform_thread_sync.py (2)
221-225: Make the ordering assertion robust (avoid brittle string index).Indexing on the pretty-printed TIR can break with harmless printer changes. Guard the substrings first and prefer structural checks.
Apply this minimal hardening:
- s = str(mod) - assert 'T.tvm_storage_sync("shared.dyn")' in s - # Ensure the sync appears before the unrolled loop - assert s.index('T.tvm_storage_sync("shared.dyn")') < s.index("for i in T.unroll(8)") + s = str(mod) + assert 'T.tvm_storage_sync("shared.dyn")' in s + assert "for i in T.unroll(8)" in s + # Ensure the sync appears before the unrolled loop + assert s.index('T.tvm_storage_sync("shared.dyn")') < s.index("for i in T.unroll(8)")Optionally, switch to a structural check to assert relative order without relying on strings (can help avoid future flakes). I can provide a small TIR visitor that validates the Evaluate(tvm_storage_sync) precedes the For(unroll) in seq order.
196-217: Tidy-ups: avoid magic constant and use canonical Op namespace.
- Replace 98304 with a self-documenting product.
- Prefer tvm.ir.Op.get over tvm.tir.op.Op.get for consistency.
- def func(): - buf_dyn_shmem = T.alloc_buffer((98304,), "uint8", scope="shared.dyn") + def func(): + # 384 threads * 256 bytes per thread + buf_dyn_shmem = T.alloc_buffer((384 * 256,), "uint8", scope="shared.dyn") @@ - T.call_intrin( + T.call_intrin( "handle", - tvm.tir.op.Op.get("tl.ptx_stmatrix"), + tvm.ir.Op.get("tl.ptx_stmatrix"), T.int32(0), T.int32(4), T.tvm_access_ptr( T.type_annotation("uint8"), buf_dyn_shmem.data, off, - 98304 - off, + 384 * 256 - off, 2, ), T.int32(2), ))format.sh (2)
56-71: Harden get_merge_base to survive fetch/network issues and ensure non-empty value.
set -ewon’t abort on a failing command substitution, leaving MERGE_BASE empty and altering behavior. Add graceful fallbacks.get_merge_base() { UPSTREAM_REPO="https://github.com/tile-ai/tilelang" - if git ls-remote --exit-code "${UPSTREAM_REPO}" main &>/dev/null; then - # First try to use the upstream repository directly - MERGE_BASE="$(git fetch "${UPSTREAM_REPO}" main &>/dev/null && git merge-base FETCH_HEAD HEAD)" - elif git show-ref --verify --quiet refs/remotes/origin/main; then + if git ls-remote --exit-code "${UPSTREAM_REPO}" main &>/dev/null; then + # First try to use the upstream repository directly + if git fetch "${UPSTREAM_REPO}" main &>/dev/null; then + MERGE_BASE="$(git merge-base FETCH_HEAD HEAD || true)" + fi + fi + if [[ -z "${MERGE_BASE}" ]] && git show-ref --verify --quiet refs/remotes/origin/main; then # Fall back to origin/main if available BASE_BRANCH="origin/main" - MERGE_BASE="$(git merge-base "${BASE_BRANCH}" HEAD)" - else + MERGE_BASE="$(git merge-base "${BASE_BRANCH}" HEAD || true)" + fi + if [[ -z "${MERGE_BASE}" ]]; then # Last resort, use local main BASE_BRANCH="main" - MERGE_BASE="$(git merge-base "${BASE_BRANCH}" HEAD)" + MERGE_BASE="$(git merge-base "${BASE_BRANCH}" HEAD || true)" + fi + if [[ -z "${MERGE_BASE}" ]]; then + # Absolute fallback: current HEAD + MERGE_BASE="$(git rev-parse HEAD)" fi echo "${MERGE_BASE}" }
136-149: Mirror safe file handling for clang-tidy changed-set.Use arrays and include untracked C/C++ sources to catch new files before the first commit.
- # Get changed C/C++ files - CHANGED_FILES="$(git diff --name-only --diff-filter=ACM "${MERGE_BASE}" -- '*.c' '*.cc' '*.cpp' '*.h' '*.hpp' 2>/dev/null || true)" - - if [[ -n "${CHANGED_FILES}" ]]; then - echo "Running clang-tidy on changed files:" - echo "${CHANGED_FILES}" - # Convert newline-separated files to space-separated and run clang-tidy once - CHANGED_FILES_SPACE="$(echo "${CHANGED_FILES}" | tr '\n' ' ')" - run-clang-tidy -j 64 ${CHANGED_FILES_SPACE} -p build -fix + # Get changed + untracked C/C++ files + mapfile -t CHANGED_CPP < <( \ + { git diff --name-only --diff-filter=ACMR "${MERGE_BASE}" -- '*.c' '*.cc' '*.cpp' '*.h' '*.hpp' 2>/dev/null || true; \ + git ls-files --others --exclude-standard -- '*.c' '*.cc' '*.cpp' '*.h' '*.hpp'; } \ + | sed '/^\s*$/d' | sort -u ) + + if ((${#CHANGED_CPP[@]} > 0)); then + echo "Running clang-tidy on changed files:" + printf '%s\n' "${CHANGED_CPP[@]}" + run-clang-tidy -j 64 "${CHANGED_CPP[@]}" -p build -fix else echo "No C/C++ files changed. Skipping clang-tidy." fiOptional: add
-Mto enable rename detection if you want to treat pure renames as changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
format.sh(3 hunks)testing/python/transform/test_tilelang_transform_thread_sync.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
testing/python/transform/test_tilelang_transform_thread_sync.py (4)
tilelang/language/ast/ir.py (5)
func(1343-1356)alloc_buffer(441-508)launch_thread(1208-1240)unroll(765-793)evaluate(1319-1331)tilelang/language/tir/op.py (3)
call_intrin(120-145)tvm_access_ptr(651-676)type_annotation(635-648)src/transform/thread_storage_sync.cc (18)
op(421-433)op(421-421)op(479-493)op(479-479)op(524-530)op(524-524)op(531-537)op(531-531)op(538-556)op(538-538)op(558-597)op(558-558)op(612-631)op(612-612)op(682-701)op(682-682)ThreadSync(837-851)ThreadSync(837-837)tilelang/transform/__init__.py (1)
ThreadSync(128-141)
Comment on lines
+93
to
+103
| # Collect changed files (committed since merge-base + current worktree) | ||
| CHANGED_FILES="$(git diff --name-only --diff-filter=ACM "${MERGE_BASE}" 2>/dev/null || true)" | ||
| if [[ -n "${CHANGED_FILES}" ]]; then | ||
| echo "Running pre-commit on changed files:" | ||
| echo "${CHANGED_FILES}" | ||
| # Convert newline-separated files to space-separated and run pre-commit once | ||
| CHANGED_FILES_SPACE="$(echo "${CHANGED_FILES}" | tr '\n' ' ')" | ||
| python3 -m pre_commit run --files ${CHANGED_FILES_SPACE} | ||
| else | ||
| echo "No files changed relative to merge base and worktree. Skipping pre-commit." | ||
| fi |
There was a problem hiding this comment.
Changed-files list misses untracked files and is unsafe for spaces.
git diff "${MERGE_BASE}"does not include untracked files; newly created files won’t be formatted.${CHANGED_FILES_SPACE}relies on word splitting; breaks on spaces or glob characters.
Use an array and include untracked files:
- # Collect changed files (committed since merge-base + current worktree)
- CHANGED_FILES="$(git diff --name-only --diff-filter=ACM "${MERGE_BASE}" 2>/dev/null || true)"
- if [[ -n "${CHANGED_FILES}" ]]; then
- echo "Running pre-commit on changed files:"
- echo "${CHANGED_FILES}"
- # Convert newline-separated files to space-separated and run pre-commit once
- CHANGED_FILES_SPACE="$(echo "${CHANGED_FILES}" | tr '\n' ' ')"
- python3 -m pre_commit run --files ${CHANGED_FILES_SPACE}
+ # Collect changed (tracked) and untracked files vs merge-base + worktree
+ mapfile -t CHANGED_FILES_ARR < <( \
+ { git diff --name-only --diff-filter=ACMR "${MERGE_BASE}" 2>/dev/null || true; \
+ git ls-files --others --exclude-standard; } \
+ | sed '/^\s*$/d' | sort -u )
+ if ((${#CHANGED_FILES_ARR[@]} > 0)); then
+ echo "Running pre-commit on changed files:"
+ printf '%s\n' "${CHANGED_FILES_ARR[@]}"
+ python3 -m pre_commit run --files "${CHANGED_FILES_ARR[@]}"
else
echo "No files changed relative to merge base and worktree. Skipping pre-commit."
fiNotes:
- Adds untracked via
git ls-files --others --exclude-standard. - Uses an array to avoid word-splitting bugs and handles odd filenames safely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Collect changed files (committed since merge-base + current worktree) | |
| CHANGED_FILES="$(git diff --name-only --diff-filter=ACM "${MERGE_BASE}" 2>/dev/null || true)" | |
| if [[ -n "${CHANGED_FILES}" ]]; then | |
| echo "Running pre-commit on changed files:" | |
| echo "${CHANGED_FILES}" | |
| # Convert newline-separated files to space-separated and run pre-commit once | |
| CHANGED_FILES_SPACE="$(echo "${CHANGED_FILES}" | tr '\n' ' ')" | |
| python3 -m pre_commit run --files ${CHANGED_FILES_SPACE} | |
| else | |
| echo "No files changed relative to merge base and worktree. Skipping pre-commit." | |
| fi | |
| # Collect changed (tracked) and untracked files vs merge-base + worktree | |
| mapfile -t CHANGED_FILES_ARR < <( \ | |
| { git diff --name-only --diff-filter=ACMR "${MERGE_BASE}" 2>/dev/null || true; \ | |
| git ls-files --others --exclude-standard; } \ | |
| | sed '/^\s*$/d' | sort -u ) | |
| if ((${#CHANGED_FILES_ARR[@]} > 0)); then | |
| echo "Running pre-commit on changed files:" | |
| printf '%s\n' "${CHANGED_FILES_ARR[@]}" | |
| python3 -m pre_commit run --files "${CHANGED_FILES_ARR[@]}" | |
| else | |
| echo "No files changed relative to merge base and worktree. Skipping pre-commit." | |
| fi |
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.
This pull request updates the
format.shscript to improve how pre-commit checks are run, especially when the workspace has uncommitted changes, and adds a new test for thread synchronization in dynamic shared memory for CUDA intest_tilelang_transform_thread_sync.py. The most important changes are grouped below by theme.Pre-commit workflow improvements in
format.sh:Testing enhancements for CUDA thread synchronization:
test_sync_shared_dyn_stmatrix_loop_hoisttotest_tilelang_transform_thread_sync.py, which verifies thatThreadSync("shared.dyn")correctly inserts a synchronization barrier before an unrolled loop using dynamic shared memory and thetl.ptx_stmatrixintrinsic. This test only runs if CUDA is available.Summary by CodeRabbit
Chores
Tests