Skip to content

Feat: internal 1d and 2d ring attention kernel - #472

Draft
eltsai wants to merge 1 commit into
mainfrom
internal-perm
Draft

Feat: internal 1d and 2d ring attention kernel#472
eltsai wants to merge 1 commit into
mainfrom
internal-perm

Conversation

@eltsai

@eltsai eltsai commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

@eltsai
eltsai requested a review from entrpn as a code owner September 3, 2026 18:51
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

@eltsai
eltsai marked this pull request as draft September 3, 2026 18:51
@eltsai eltsai self-assigned this Sep 3, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an internal ring attention kernel for MaxDiffusion, which performs KV permutation directly within the Pallas kernel using remote DMA to improve performance. The changes include the implementation of the new kernel, updates to kernel registration and configuration, and a refined VMEM ceiling model for tile size grid search to better optimize tile selection. The review identified a critical bug in the DMA descriptor handling within the new kernel and noted redundant entries in the kernel registration list, both of which require attention.

Comment on lines +191 to +196
@pl.when(is_first_hop)
def _prime():
for dma in _local_load(0, 0):
dma.start()
for dma in _local_load(0, 0):
dma.wait()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In _prime(), _local_load(0, 0) is called twice, which constructs two separate sets of DMA descriptors. The first set is started but never waited on, and the second set is waited on but never started. This can lead to compilation issues, deadlocks, or undefined behavior in Pallas. Store the descriptors in a local variable and reuse them for both .start() and .wait().

Suggested change
@pl.when(is_first_hop)
def _prime():
for dma in _local_load(0, 0):
dma.start()
for dma in _local_load(0, 0):
dma.wait()
@pl.when(is_first_hop)
def _prime():
dmas = _local_load(0, 0)
for dma in dmas:
dma.start()
for dma in dmas:
dma.wait()

Comment on lines +2653 to +2658
"ulysses_ring_custom_iperm",
"ulysses_ring_custom_iperm_fixed_m",
"ulysses_ring_custom_iperm_fixed_m_nocond",
"ulysses_ring_custom_iperm_fixed_m_nocond",
"ulysses_ring_custom_iperm_fixed_m",
"ulysses_ring_custom_iperm_fixed_m_nocond",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There are duplicate and redundant attention kernel names in the in tuple. Specifically, "ulysses_ring_custom_iperm_fixed_m_nocond" is repeated multiple times, and "ulysses_ring_custom_iperm_fixed_m" is also duplicated. These redundant entries should be removed to keep the code clean and maintainable.

Suggested change
"ulysses_ring_custom_iperm",
"ulysses_ring_custom_iperm_fixed_m",
"ulysses_ring_custom_iperm_fixed_m_nocond",
"ulysses_ring_custom_iperm_fixed_m_nocond",
"ulysses_ring_custom_iperm_fixed_m",
"ulysses_ring_custom_iperm_fixed_m_nocond",
"ulysses_ring_custom_iperm",
"ulysses_ring_custom_iperm_fixed_m",
"ulysses_ring_custom_iperm_fixed_m_nocond",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant