Repository navigation
[Enhancement] Add missing fence_barrier_init primitive after mbarrier init - #1121
Conversation
WalkthroughThese changes introduce GPU barrier initialization support for Hopper architecture. A new CUDA barrier initialization function is added to the barrier header and integrated into the lowering transformation to emit synchronization before shared memory storage operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes The changes consist of straightforward additions: a new utility function and a simple insertion point for synchronization. The modifications follow existing patterns and are homogeneous in nature (both related to barrier initialization). 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 |
|
👋 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! 🚀 |
There was a problem hiding this comment.
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 (1)
src/op/builtin.h (1)
15-21: Relocate the misplaced documentation comment.This documentation block describes
ptx_fence_barrier_init()but is placed before the namespace declaration. It should be moved to lines 207-212, immediately before the actual function declaration.Apply this diff to relocate the comment:
-/*! - * \brief Create the TVM intrinsic that initializes a PTX fence barrier. - * - * Initializes a PTX fence-style barrier used to coordinate asynchronous memory - * operations (for example, TMA/TMA_STORE). Returns the Op representing this - * intrinsic for use in TIR lowering and code generation. - * - */ namespace tvm {And add it before the function declaration:
+/*! + * \brief Create the TVM intrinsic that initializes a PTX fence barrier. + * + * Initializes a PTX fence-style barrier used to coordinate asynchronous memory + * operations (for example, TMA/TMA_STORE). Returns the Op representing this + * intrinsic for use in TIR lowering and code generation. + */ const Op &ptx_fence_barrier_init();
🧹 Nitpick comments (1)
src/transform/lower_hopper_intrin.cc (1)
86-95: Op is properly registered; comment formatting remains optional.The verification confirms that
ptx_fence_barrier_init()is correctly registered viaTIR_DEFINE_TL_BUILTINinsrc/op/builtin.ccand properly declared insrc/op/builtin.h. The fence insertion logic is sound and follows FlashAttention best practices.The comment formatting suggestion remains optional but valid if you prefer improved readability:
- // Note from FlashAttention: - // Helps with visibility of barrier init operations across warps / - // cta / cluster Available as a separate function so as to batch - // inits across barriers and fence once Note : It must be composed - // with an appropriate sync instruction with the right scope to - // ensure visibility eg. __syncthreads() or a cluster_arrive() + - // cluster_wait() + // Note from FlashAttention: + // Helps with visibility of barrier init operations across warps, + // CTA, and cluster. Available as a separate function so as to + // batch inits across barriers and fence once. + // Note: It must be composed with an appropriate sync instruction + // with the right scope to ensure visibility (e.g., __syncthreads() + // or cluster_arrive() + cluster_wait()).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/op/builtin.h(1 hunks)src/tl_templates/cuda/barrier.h(1 hunks)src/transform/lower_hopper_intrin.cc(1 hunks)
🔇 Additional comments (2)
src/op/builtin.h (1)
506-507: LGTM!The blank line improves visual separation between intrinsic declarations.
src/tl_templates/cuda/barrier.h (1)
136-138: PTX instruction syntax verified as correct.The instruction syntax
fence.mbarrier_init.release.clusteris correct for sm_90+. The code infence_barrier_init()matches the official NVIDIA PTX ISA specification. No issues found.
|
cc @cherichy |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…er init (tile-ai#1121) * [Enhancement] Add missing primitive after mbarrier init * lint
This pull request introduces improvements to CUDA barrier synchronization primitives and their usage in the codebase, particularly to enhance memory visibility and synchronization across warps and clusters. The most important changes are grouped below:
CUDA Barrier Synchronization Enhancements
fence_barrier_initinbarrier.hto issue thefence.mbarrier_init.release.clusterPTX instruction, improving memory barrier initialization and visibility across clusters.LowerHopperIntrinto insert a call toptx_fence_barrier_initafter initializing memory barriers, following best practices from FlashAttention for proper synchronization.Operator Intrinsic Updates
increase_descriptor_offsettobuiltin.h, supporting descriptor offset management in kernel code.Summary by CodeRabbit