-
Notifications
You must be signed in to change notification settings - Fork 449
[RaceChecker] RaceChecker report warning rather than error for backward compatibility #1620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
👋 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! 🚀 |
📝 WalkthroughWalkthroughAdds a pass config option and attribute key to disable data-race checks, and relaxes/softens the parallel-loop data-race verifier by expanding safe buffer scopes and turning a fatal error into a warning. Changes
Sequence Diagram(s)(omitted — changes are small and do not introduce a multi-component sequential flow that requires visualization) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/transform/verify_parallel_loop.cc:
- Around line 231-237: The pass currently always runs the data-race verifier;
update the pass entry (where you access PassContext) to read
PassKey.TL_DISABLE_DATA_RACE_CHECK from the PassContext and skip the verifier
when the flag is set: query the PassContext for the TL_DISABLE_DATA_RACE_CHECK
option at the start of the verifier routine and, if true, return early (before
invoking the analyzer/z3_prover logic and before emitting the LOG(WARNING)
message referencing op->buffer, op->indices, failed_vars, and failed_var_expr)
so the disable flag actually disables the check.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/op/builtin.ccsrc/op/builtin.hsrc/transform/verify_parallel_loop.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T09:47:46.474Z
Learnt from: kurisu6912
Repo: tile-ai/tilelang PR: 794
File: tilelang/transform/add_bufstore_wrapper.py:30-33
Timestamp: 2025-09-12T09:47:46.474Z
Learning: In TVM's PyStmtExprMutator, visit_block_ methods typically call super().visit_block_(op) to process child nodes and update internal state, but return the original op when the block itself doesn't need transformation. The pattern `return op` is correct for blocks that serve as containers where mutations happen at deeper levels.
Applied to files:
src/transform/verify_parallel_loop.cc
🧬 Code graph analysis (1)
src/transform/verify_parallel_loop.cc (1)
src/transform/simplify.cc (16)
op(95-104)op(95-95)op(105-108)op(105-105)op(109-112)op(109-109)op(113-136)op(113-113)op(177-180)op(177-177)op(181-184)op(181-181)op(314-324)op(314-314)op(326-337)op(326-326)
🔇 Additional comments (3)
src/transform/verify_parallel_loop.cc (1)
197-198: LGTM: Thread-local buffer scope exclusion is correct.Expanding the skip condition to treat
local.varandlocalscoped buffers as non-racy is appropriate, as thread-local buffers cannot have data races between parallel threads.src/op/builtin.h (1)
64-65: LGTM: Constant declaration follows codebase conventions.The new attribute key
kDisableDataRaceCheckis correctly declared and follows the same pattern as other configuration constants in the file.src/op/builtin.cc (1)
39-39: LGTM: Pass config option registration is correct.The registration of
kDisableDataRaceCheckas a Bool config option follows the correct pattern and is consistent with other option registrations in this file.Note: The critical issue that this option is not actually checked in the
VerifyParallelLooppass has been flagged in the review ofsrc/transform/verify_parallel_loop.cc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/transform/verify_parallel_loop.cc:
- Around line 231-238: The warning message emitted in verify_parallel_loop (the
LOG(WARNING) that concatenates op->buffer, op->indices, failed_vars and
analyzer.z3_prover.GetModel(failed_var_expr)) has two text issues: add a space
between the closing backtick and "is written" so the output reads "...` is
written by multiple threads..." and rephrase the final clause "to pass key to
pass key to disable this check" to a clearer form such as "pass
PassKey.TL_DISABLE_DATA_RACE_CHECK in the pass configuration to disable this
check"; update the LOG(WARNING) string accordingly while keeping the same
concatenation of op->buffer, op->indices, failed_vars and the model output.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/transform/verify_parallel_loop.cc
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T09:47:46.474Z
Learnt from: kurisu6912
Repo: tile-ai/tilelang PR: 794
File: tilelang/transform/add_bufstore_wrapper.py:30-33
Timestamp: 2025-09-12T09:47:46.474Z
Learning: In TVM's PyStmtExprMutator, visit_block_ methods typically call super().visit_block_(op) to process child nodes and update internal state, but return the original op when the block itself doesn't need transformation. The pattern `return op` is correct for blocks that serve as containers where mutations happen at deeper levels.
Applied to files:
src/transform/verify_parallel_loop.cc
🧬 Code graph analysis (1)
src/transform/verify_parallel_loop.cc (1)
src/transform/simplify.cc (16)
op(95-104)op(95-95)op(105-108)op(105-105)op(109-112)op(109-109)op(113-136)op(113-113)op(177-180)op(177-177)op(181-184)op(181-181)op(314-324)op(314-314)op(326-337)op(326-326)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test for Python 3.12 with Nightly-ROCm-7.1 (on self-hosted-amd)
- GitHub Check: Test for Python 3.12 with Metal (on macos-latest)
🔇 Additional comments (1)
src/transform/verify_parallel_loop.cc (1)
197-198: The scope assumptions in lines 197-198 are correct. The"local"and"local.var"scopes represent per-thread storage (registers and thread-local variables) and are inherently thread-safe. This is validated by the codebase pattern wherelower_tile_op.ccexplicitly documents that loops manipulating only local buffers indicate register usage, which justifies skipping thread binding and, by extension, race detection.
as title
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.