Skip to content

[Metal:Bugfix] Respect attention kv_cache flag#4556

Merged
wangzhaode merged 1 commit into
masterfrom
sync/20260622-1144
Jun 22, 2026
Merged

[Metal:Bugfix] Respect attention kv_cache flag#4556
wangzhaode merged 1 commit into
masterfrom
sync/20260622-1144

Conversation

@wangzhaode

Copy link
Copy Markdown
Collaborator

Discussed-in: Merge-Request 28103114 , URL: https://code.alibaba-inc.com/AliNN/AliNNPrivate/codereview/28103114
GitOrigin-RevId: 4f99ad5b21383d1b845ff53af7472a66f51af4da

Description

Module

Type

  • Feature
  • Bugfix
  • Perf
  • Refact
  • Style
  • Doc
  • Test
  • Chore

Checklist

  • Commit message follows [Module:Type] Description format
  • Code compiles without errors
  • Tested on relevant platform(s)
  • No unrelated format or style changes included

Discussed-in: Merge-Request 28103114 , URL: https://code.alibaba-inc.com/AliNN/AliNNPrivate/codereview/28103114
GitOrigin-RevId: 4f99ad5b21383d1b845ff53af7472a66f51af4da
@wangzhaode
wangzhaode merged commit 9070bc7 into master Jun 22, 2026
14 checks passed

@github-actions github-actions 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.

💡 来自 MNN 助手的智能建议

if (mKVCache && bn->getMetaPtr() == mMeta && mMeta != nullptr) {
exe->mKVCacheManager = mKVCacheManager;
}
*dst = exe;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

onClonemKVCacheManager 可能为空的潜在风险

mKVCachetrue 但条件 bn->getMetaPtr() == mMeta && mMeta != nullptr 不满足时,克隆出的对象将拥有 mKVCache = truemKVCacheManager = nullptr。如果后续 onResize 没有重新创建 mKVCacheManager,则在 onEncode 中解引用 mKVCacheManager 将导致空指针崩溃。

建议:

  1. 在使用 mKVCacheManager 的所有路径中添加空指针检查(如 mKVCache && mKVCacheManager);
  2. 或者在 onClone 中确保当 mKVCachetrue 但无法共享 manager 时,在 onResize 中为新对象创建独立的 mKVCacheManager

此外,构造函数参数已变更为 (Backend*, bool kvCache, bool outputC4, float, shared_ptr),请确认 .cpp 中的实现是否正确初始化了 mKVCache

bool mKVCache = true;
std::shared_ptr<MetalKVCacheManager> mKVCacheManager = nullptr;
float mAttnScale = 0.0f;
float mScale;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

mKVCache 默认值为 true 可能引起意外行为

bool mKVCache = true;

mKVCache 默认初始化为 true,这意味着如果构造函数因某种原因未正确设置此成员(或者未来有人新增不完整的构造函数路径),KV Cache 功能会被默认启用,可能导致不必要的内存分配或执行不兼容的代码路径。建议将默认值设为 false,强制显式启用,符合安全默认原则(fail-safe)。

同时,mKVCachemKVCacheManager 的状态应保持一致——当 mKVCachefalse 时,应确保 mKVCacheManager 始终为 nullptr,且所有使用 mKVCacheManager 的代码路径都受 mKVCache 守护,避免状态不一致。

} else if (mKVCache) {
qkPrefillKeys.emplace_back("DEFAULT_MASK");
if (seq_len > 1) {
qkKeys.emplace_back("DEFAULT_MASK");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

else 改为 else if (mKVCache) 后,当 !mKVCache && !mHasMask 时,既不会添加 SET_MASK 也不会添加 DEFAULT_MASK,这意味着非 KV Cache 且无显式 mask 的情况下完全没有 mask。如果这是用于双向注意力(如 encoder),则无 mask 是正确的;但如果某些非 KV Cache 场景仍需要因果 mask,则会引入 bug。请确认 mKVCache=false 的所有场景是否确实不需要 DEFAULT_MASK,建议添加注释说明设计意图。


if (nullptr == mMeta || mMeta->previous == mMeta->remove) {
mKVCacheManager->onClear();
mKVCacheManager->onAlloc(mMeta, mCurrentKvLen);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

此代码块有几个问题需要关注:

  1. mTempQK 缺少空指针检查:对 mTempK/mTempV 做了 nullptr 检查,但 mTempQK->length(0)mTempQK->length(1) 调用前没有检查 mTempQK 是否为空。如果 mTempQK 未初始化,会导致崩溃。同样 mTempSoftMax 也缺少检查。建议保持一致性,添加空指针检查。

  2. mCurrentKvLen 的有效性:在非 KV Cache 路径中,mKvSeqLen = mCurrentKvLen。需确认 mCurrentKvLen 在进入此路径前已被正确赋值(应等于当前序列长度),否则 mKvSeqLen 可能是错误值,影响后续计算。

  3. mTempK/mTempV 创建为一维 Tensor:使用 Tensor::createDevice<float>({keySize}) 创建了一维 Tensor,但后续 onEncode 中作为 K/V tensor 使用时,metal kernel 可能期望特定的多维布局。请确认 metal shader 能正确处理一维 Tensor 的 buffer 绑定。

  4. 错误信息不够详细MNN_ERROR%d 打印 res(bool 类型),只会输出 0 或 1,不太有助于诊断哪个 buffer 分配失败。建议分别检查每个 onAcquireBuffer 的返回值并输出更具体的错误信息。

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