Add 8 priority human optimized kernels and SWA into JaxBench level 2 - #101
shangkunwang01 wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces several new JAXBench workloads, including implementations for Megablox GMM, Flash Attention, GQA Attention, and MLA Attention. The review feedback highlights a critical issue regarding the order of operations for attention bias and scaling in the Flash Attention kernel, which affects numerical correctness. Other feedback points out opportunities for code cleanup, such as improving type hints, refactoring logic for clarity, and fixing import placement to follow PEP 8 guidelines.
| if ab_tile_ref is not None: | ||
| ab = ab_tile_ref[ | ||
| (*batch_idx, pl.dslice(None), pl.dslice(start_k, block_k)) | ||
| ].astype(jnp.float32) | ||
| s += ab | ||
|
|
||
| if sm_scale != 1.0: | ||
| s *= sm_scale |
There was a problem hiding this comment.
The order of applying attention bias and sm_scale seems incorrect. Currently, the bias is added before scaling: (Q @ K.T + bias) * sm_scale. The standard implementation of attention applies the scale factor before adding the bias: (Q @ K.T * sm_scale) + bias. This should be changed to ensure correctness, especially if the model was trained with the standard attention implementation. This issue appears in multiple places in this file (including the reference implementation and backward pass) and should be fixed consistently.
| if ab_tile_ref is not None: | |
| ab = ab_tile_ref[ | |
| (*batch_idx, pl.dslice(None), pl.dslice(start_k, block_k)) | |
| ].astype(jnp.float32) | |
| s += ab | |
| if sm_scale != 1.0: | |
| s *= sm_scale | |
| if sm_scale != 1.0: | |
| s *= sm_scale | |
| # Add attention bias if needed. | |
| # TODO(tanburn) Should the attention bias be added before or after | |
| # multiplication by sm_scale? | |
| if ab_tile_ref is not None: | |
| ab = ab_tile_ref[ | |
| (*batch_idx, pl.dslice(None), pl.dslice(start_k, block_k)) | |
| ].astype(jnp.float32) | |
| s += ab |
There was a problem hiding this comment.
No fix. Keep consistent with the original implementation.
| repeats, rem = divmod(block_k, NUM_LANES) | ||
| if rem: | ||
| raise NotImplementedError( | ||
| f"kv block size must be a multiple of {NUM_LANES}" | ||
| ) | ||
| q_segment_ids = jnp.tile( | ||
| q_segment_ids_tile_ref[batch_idx[0]], (1, repeats) | ||
| ) |
There was a problem hiding this comment.
The order of applying attention bias and sm_scale seems incorrect. Currently, the bias is added before scaling: (Q @ K.T + bias) * sm_scale. The standard implementation of attention applies the scale factor before adding the bias: (Q @ K.T * sm_scale) + bias. This should be changed to ensure correctness, especially if the model was trained with the standard attention implementation. This issue appears in multiple places in this file and should be fixed consistently.
if sm_scale != 1.0:
s *= sm_scale
# Add attention bias if needed.
# TODO(tanburn) Should the attention bias be added before or after
# multiplication by sm_scale?
if ab_tile_ref is not None:
ab = ab_tile_ref[
(*batch_idx, pl.dslice(None), pl.dslice(start_k, block_k))
].astype(jnp.float32)
s += abThere was a problem hiding this comment.
Same as above.
| return tiles, rem | ||
|
|
||
|
|
||
| GroupMetadata = Any # TODO(enriqueps): Clean this up and use a namedtuple |
There was a problem hiding this comment.
Same as above.
| partial_tile_mask = jnp.logical_or( | ||
| (group_offsets[:-1] % tm) == 0, group_sizes == 0 | ||
| ) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Same as above.
| group_ids = jnp.roll(group_ids, shift=-first_tile_in_shard, axis=0) | ||
| m_tile_ids = jnp.roll(m_tile_ids, shift=-first_tile_in_shard, axis=0) |
There was a problem hiding this comment.
As the TODO on line 249 suggests, using jnp.roll here can be inefficient as it may involve unnecessary data movement on the accelerator. This logic could be moved inside the Pallas kernel by passing first_tile_in_shard as an argument and adjusting the indices within the kernel. This would avoid the overhead of jnp.roll.
There was a problem hiding this comment.
Same as above.
| import time | ||
| inputs = create_inputs() | ||
| fn = jax.jit(workload) | ||
| for _ in range(num_warmup): | ||
| out = fn(*inputs) | ||
| out.block_until_ready() | ||
| times = [] | ||
| for _ in range(num_iters): | ||
| t0 = time.perf_counter() | ||
| out = fn(*inputs) | ||
| out.block_until_ready() | ||
| times.append(time.perf_counter() - t0) | ||
| import numpy as np |
There was a problem hiding this comment.
Imports should be at the top of the file, as per PEP 8 guidelines. import time and import numpy as np should be moved from inside the benchmark function to the top-level of the module. This improves code organization and avoids re-importing modules. This issue is present in most of the new baseline.py files in this pull request.
| new_kv_c: jax.Array, # [max_num_tokens, actual_lkv_dim] | ||
| new_k_pe: jax.Array, # [max_num_tokens, actual_r_dim] | ||
| cache_kv: jax. | ||
| Array, # [total_num_pages, page_size_per_kv_packing, kv_packing, lkv_dim] |
There was a problem hiding this comment.
The type hint comment for cache_kv has a typo. The last dimension should be the combined and aligned key-value dimension, lkv_dim + r_dim, not just lkv_dim.
| Array, # [total_num_pages, page_size_per_kv_packing, kv_packing, lkv_dim] | |
| Array, # [total_num_pages, page_size_per_kv_packing, kv_packing, align_to(lkv_dim + r_dim, 128)] |
There was a problem hiding this comment.
Keep it as it is to be consistent with the original code.
NinaCai
left a comment
There was a problem hiding this comment.
Could you rename swa as 53p_DeepSeek_V4_MLA_SWA in both JaxBench and jaxbench_adapted_dataset?
If you plan to work on CSA and HCA as well. Could you follow 51p_DeepSeek_V4_CSA and 52p_DeepSeek_V4_HCA?
ebf6a45 to
8f170d3
Compare
Updated. |
MaxKernel/evaluation/jaxbench_adapted_dataset/verify_with_jaxbench.py