Add ttri, thistogram#976
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
| } | ||
| } | ||
| if (auto ttri = dyn_cast<pto::TTriOp>(op)) { | ||
| attrs.emplace_back("upper_or_lower", std::to_string(ttri.getUpperOrLower())); |
There was a problem hiding this comment.
IR形态允许upper_or_lower是空值，这里需要做空值保护
There was a problem hiding this comment.
def TTriOp : PTO_TOp<"ttri", [
PTO_DpsInitOpInterface,
OpPipeInterface,
DeclareOpInterfaceMethods
]> {
let arguments = (ins
AnyInteger:$diagonal,
PTODpsType:$dst,
DefaultValuedAttr<I32Attr, "0">:$upperOrLower
); 参数默认值为0而非空
| ### 8.1.14 Tile compute quick reference | ||
| ### 8.1.14 Triangular mask generation | ||
|
|
||
| #### `pto.tile.tri(diagonal: IndexLike, dst: Tile, *, upper_or_lower: int | None = None) -> None` |
There was a problem hiding this comment.
建议DSL侧upper_or_lower改成shape="upper"|"lower"（默认为lower）语义会更清晰，
There was a problem hiding this comment.
已更改为既支持int类型又支持字符串，文档同步修改
def is_lower(upper_or_lower=0, **):
if isinstance(upper_or_lower, str):
return upper_or_lower in ("0", "lower")
return int(upper_or_lower) == 0
def ttri(diagonal, dst, *, upper_or_lower="lower"):
"""pto.ttri ins(diagonal) outs(dst)."""
if isinstance(upper_or_lower, str):
if upper_or_lower == "lower":
upper_or_lower = 0
elif upper_or_lower == "upper":
upper_or_lower = 1
else:
raise ValueError(
f"upper_or_lower must be 'lower' or 'upper', got {upper_or_lower!r}"
)
.......
| out_tile = pto.alloc_tile(shape=[4, 8], dtype=pto.f32, valid_shape=[4, 4]) | ||
|
|
||
| # Lower triangular, diagonal=0 → dst[i,j]=1 where j<=i | ||
| pto.tile.tri(pto.i32(0), out_tile, upper_or_lower=0) |
There was a problem hiding this comment.
这里实际上不需要写成pto.i32(0)，因为类型可以通过out_tile类型推导出来
| context_attrs=context_attrs or {}, | ||
| **tile_specs, | ||
| ).mlir_text() | ||
| return mlir_text |
Zhendong404
left a comment
Zhendong404
left a comment
There was a problem hiding this comment.
PR #976 Review:Add ttri、thistogram 模板与测试
整体结构没问题（C++ 上下文属性 + Python 模板 + ST 测试三件套齐全），但有以下缺陷和风险，按严重程度排序：
高风险
1. TTriOp 的 upper_or_lower 属性未做空值保护（C++ 侧）
ExpandTileOp.cpp:9-11 和 InsertTemplateAttributes.cpp:29-31 中直接调用 ttri.getUpperOrLower()。而 Python 侧 ttri(..., upper_or_lower=None) 是允许的（文档也写了 "If None, the backend default applies")。对比同 PR 中 THistogramOp 对 byte 做了 getByteAttr() 非空判断——如果 upper_or_lower 是 Optional 属性且用户传 None，这里会 assert/崩溃或未定义行为。要么像 byte 一样加保护，要么在 _ops.ttri 里强制拒绝 None。
2. histogram 模板存在越界读取风险
thistogram.py 中列循环固定步长 _ELEM_PER_REPEAT_B8 = 256，而 vldsx2 / _deintlv_u32_bytes 是无 mask 的整向量加载。当 valid_cols 不是 256 的倍数时（测试里就有 cols=100/192/912)，最后一次迭代会读超出本行物理列、甚至超出整个 tile 末尾（最后一行时）。超出的数据虽被 elem_mask 屏蔽不影响结果正确性，但对 UB 末尾的 tile 是 OOB 读，有硬件异常风险，且行为依赖 UB 分配器的 padding，很脆弱。建议约束要求物理列对齐到 256，或末次迭代改用带 mask 的加载。
中风险
3. byte 默认值在 C++ 与 Python 约束之间不一致
C++ 侧 byte 缺省为 1（文档也说默认 1)，但模板约束 def _is_byte_0(byte=0, **_) 等的 Python 默认是 0。只要 context_attrs 里 byte 缺失（任何绕过 ExpandTileOp 默认值的一条路径）就会选错模板。默认值应统一为 1。
4. upper_or_lower / byte 缺少非法值兜底
_ops.ttri 校验了 0/1，但 tthistogram 对 byte 没有范围检查——传 byte=5 会静默走到"无模板匹配"才报错，错误信息不直观。且 ui16 时 byte 只能是 0/1，也没有校验（文档声称有约束）。
5. 文档声称的 dtype 支持面与测试不符
文档宣称 tri 支持 bf16, i8, ui8, ui16 等 9 种类型，但测试只覆盖 f32/f16/i32/i16/ui32。TRI_DTYPES 来自 NUMERIC_DTYPES，未测类型（尤其 bf16/i8 的 vbr 广播和 make_mask）是否真能用没有保障。要么补测试，要么收敛文档声明。
低风险 / 建议
6. ttri 模板两次全量写 tile（先全填 0 再覆盖 1，或反之），store 流量翻倍。可以用按行计算的 mask 一次写入完成，属于性能优化而非正确性问题。
7. daemon.py 的改动与本 PR 无关——把 return descriptor.specialize(...).mlir_text() 拆成两行，纯噪音，建议移除或单独提交。common.py 末尾删空行同理（虽然 --case 参数本身是有用的）。
8. _histogram_core 里 b16 累加器每次迭代清零重建，依赖"每次 flush 前单 bin 最多 256 计数不溢出 65535"这一隐含不变量，建议加注释说明，否则后续有人调大 _ELEM_PER_REPEAT_B8 会引入静默溢出。
9. 小问题：template_ttri_lower 里 start_row = scalar.index_cast(pto.const(0, dtype=pto.i32)) 与 else 分支的 0 - diagonal 写法不一致，直接用 0 即可；_deintlv_u32_bytes 依赖小端字节序，建议注释说明。
|
1：默认值为1而非空值，判断不需要修改； |
2 participants
Add templates and test cases for ttri and thistogram.