-
Notifications
You must be signed in to change notification settings - Fork 450
[Enhancement] Log warnings for OOB acceses to non-global buffers #1693
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! 🚀 |
📝 WalkthroughWalkthroughThe primary visitor class in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/transform/legalize_safe_memory_access.cc (2)
51-58: Stale comment.The comment "Check if the buffer is in global scope" is outdated. The code now checks all buffers, with behavior varying based on whether the buffer is global. Consider updating it to match the corresponding comment in
VisitExpr_for consistency.Suggested fix
void VisitStmt_(const BufferStoreNode *op) final { - // Check if the buffer is in global scope + // If the buffer is in global scope, we will check its indices and add + // corresponding bound checks. + // If the buffer is in shared/local, we only log warnings if there + // are possible out-of-bounds. CheckBufferIndices(op->buffer, op->indices, /*is_load=*/false, !IsGlobalBuffer(op->buffer));
60-69: Use existing utility fromsrc/op/utils.hinstead of duplicating.An
IsGlobalBufferhelper already exists insrc/op/utils.hwith identical logic plus a defensivebuffer.defined()check that this implementation lacks. The lengthy comment is also outdated since the helper it mentions already exists.Suggested fix
Include the header and remove the local definition:
`#include` "../op/builtin.h" `#include` "../op/parallel.h" +#include "../op/utils.h" `#include` "arith/ir_mutator_with_analyzer.h"Then remove the local
IsGlobalBuffermethod (lines 60-69) and use the utility directly. The calls at lines 45 and 54 will work unchanged since the function signature is compatible.
|
@regression-perf |
Performance Regression Test ReportTriggered by: @SiriusNEO Results
Artifacts
|
For some thread programs (non-tile programs), OOB accesses to non-global buffers may also happen. For better developing these kernels, we add additional warnings if the compiler detects such cases.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.