Skip to content

Expose configurable frontend pipe slot_num#710

Open
zhangstevenunity wants to merge 1 commit into
mainfrom
codex/issue-709-slot-num
Open

Expose configurable frontend pipe slot_num#710
zhangstevenunity wants to merge 1 commit into
mainfrom
codex/issue-709-slot-num

Conversation

@zhangstevenunity
Copy link
Copy Markdown
Collaborator

Summary: add optional slot_num to frontend aic/aiv initialize_pipe ops, forward it during lowering, relax internal pipe slot_num verification to positive values, and keep local_slot_num bounded by slot_num. Tests: ninja -C build-main-wsl ptoas; llvm-lit related frontend/internal slot_num tests; llvm-lit existing tpush/tpop regression tests. Fixes #709

Copy link
Copy Markdown

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

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 optional compile-time integer attribute slot_num to the frontend pipe initialization operations (pto.aic_initialize_pipe and pto.aiv_initialize_pipe). This attribute controls the GM ring FIFO depth, which previously defaulted to fixed values of 8 or 4 depending on the communication direction (dir_mask). The changes include updating the ODS definitions, adding parsing and printing logic, enhancing verification rules to ensure slot_num is positive and local_slot_num does not exceed it, and updating lowering passes to propagate this attribute. Documentation and comprehensive LIT tests have also been added to verify correct behavior and error handling. I have no feedback to provide as there are no review comments.

@reedhecre
Copy link
Copy Markdown

reedhecre commented May 27, 2026

Codex Review

该评论由 review 机器人自动更新。

  • PR: Expose configurable frontend pipe slot_num #710 Expose configurable frontend pipe slot_num
  • Author: zhangstevenunity
  • Base/Head: main / codex/issue-709-slot-num
  • Head SHA: f82d8467ac02
  • Trigger: 检测到新的 open PR
  • Generated At: 2026-05-27T08:47:51Z
  • Status: completed

Summary

Found public contract mismatches in the updated configurable slot_num documentation.

Findings

  1. P3 Design doc still says `slot_num` is fixed by `dir_mask` docs/designs/ptoas-tpush-tpop-design.md:452

Section 4.4 still states that SLOT_NUM is fixed to 8 for dir_mask = 1/2 and 4 for dir_mask = 3, but this PR explicitly makes frontend slot_num configurable and later in the same document already says callers may override the defaults. Because this design doc is the frontend contract referenced from the IR manual, downstream generators can keep hard-coding 8/4 or reject valid IR such as the new slot_num = 2 cases.

  1. P3 IR manual describes `slot_num` as GM-only even though the new lowering also applies it on A5 docs/PTO_IR_manual.md:8350

The updated manual says slot_num controls the GM ring FIFO depth, but the new lowering threads the same frontend attribute through the A5 initialize_l2l_pipe path as well (getFrontendSlotNum() is shared and the A5 branch consumes its result). That makes the published interface misleading for A5 users: the attribute is not GM-only anymore, and its A5 behavior is now undocumented.

@zhangstevenunity zhangstevenunity marked this pull request as ready for review May 27, 2026 08:41
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.

Expose configurable GM-ring slot_num on aic/aiv_initialize_pipe (currently hardcoded to 8/4)

2 participants