Skip to content

feat: align tcvt/random/mrgsort/matmulmx with pto-isa#483

Open
HecreReed wants to merge 4 commits intohw-native-sys:mainfrom
HecreReed:codex/tcvt-random-draft
Open

feat: align tcvt/random/mrgsort/matmulmx with pto-isa#483
HecreReed wants to merge 4 commits intohw-native-sys:mainfrom
HecreReed:codex/tcvt-random-draft

Conversation

@HecreReed
Copy link
Copy Markdown
Collaborator

Summary

  • extend pto.tcvt to cover all pto-isa call forms, including optional tmp tile and optional saturation mode
  • add new A5 pto.trandom op and lower it to TRANDOM
  • switch pto.tmrgsort format2 so tmp is an ins operand, matching pto-isa
  • lower all matmul-mx variants to TMATMUL_MX(...) so generated C++ matches pto-isa

Validation

  • cmake --build build-codex --target ptoas -j8
  • cmake --build build-codex --target PTOPythonModules PTOCAPI -j8
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a3 test/basic/tcvt_emitc.pto | FileCheck ... --check-prefix=A3
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a5 test/basic/tcvt_emitc.pto | FileCheck ... --check-prefix=A5
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a3 test/basic/tcvt_e2e.pto | FileCheck ... --check-prefix=A3
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a5 test/basic/tcvt_e2e.pto | FileCheck ... --check-prefix=A5
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a3 test/basic/tmrgsort_variants_emitc.pto | FileCheck ... --check-prefix=A3
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a5 test/basic/tmatmul_mx_emitc.pto | FileCheck ... --check-prefix=A5
  • ./build-codex/tools/ptoas/ptoas --pto-arch=a5 test/basic/trandom_emitc.pto | FileCheck ... --check-prefix=A5

Notes

  • local Python runtime verification of the sample script was not completed because the available host Python does not match the built MLIR Python package ABI on this machine.

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 the SaturationMode attribute and the TRandomOp operation to the PTO dialect. It also updates TCvtOp and TMrgSortOp to include an optional tmp operand and adds saturation mode support to TCvtOp. Furthermore, matrix multiplication intrinsics have been unified under the TMATMUL_MX name. A critical bug was found in the TMrgSortOp parser where the excuted operand is parsed but not resolved into the operation's operands list.

Comment on lines 5913 to 5915
if (parser.resolveOperands(srcs, srcTypes, parser.getCurrentLocation(), result.operands) ||
parser.resolveOperand(dstOp, dstTy, result.operands) ||
parser.resolveOperand(tmpOp, tmpTy, result.operands) ||
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

critical

The excutedOp operand is parsed on line 5906 but it is not resolved into result.operands. This will cause the operation to be created without the excuted operand, leading to verification failures or incorrect code generation.

Suggested change
if (parser.resolveOperands(srcs, srcTypes, parser.getCurrentLocation(), result.operands) ||
parser.resolveOperand(dstOp, dstTy, result.operands) ||
parser.resolveOperand(tmpOp, tmpTy, result.operands) ||
if (parser.resolveOperands(srcs, srcTypes, parser.getCurrentLocation(), result.operands) ||
parser.resolveOperand(dstOp, dstTy, result.operands) ||
parser.resolveOperand(tmpOp, tmpTy, result.operands) ||
parser.resolveOperand(excutedOp, excutedTy, result.operands))
return failure();

@reedhecre
Copy link
Copy Markdown

reedhecre commented Apr 15, 2026

Codex Review

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

  • PR: feat: align tcvt/random/mrgsort/matmulmx with pto-isa #483 feat: align tcvt/random/mrgsort/matmulmx with pto-isa
  • Author: HecreReed
  • Base/Head: main / codex/tcvt-random-draft
  • Head SHA: f68598411682
  • Trigger: PR 有新提交
  • Generated At: 2026-04-15T07:51:06Z
  • Previous Head SHA: 106f3e41c54c
  • Status: completed

Summary

检查到 2 个与 correctness/contract 相关的问题:tmrgsort 在 format2 改造后的 bufferization 写集建模不完整,trandom 的 key/counter 类型约束与对外契约不一致。

Findings

  1. P2 TMrgSort format2 的 tmp 写入未同步到 BufferizableOpInterface,可能导致错误别名决策 lib/PTO/Transforms/BufferizableOpInterfaceImpl.cpp:137

本 PR 将 format2 改为 ins(srcs..., tmp) outs(dst, excuted)tmp 不再属于 dps init;但 PTOMrgSortDpsOpInterface 仍直接复用 DstBufferizableOpInterfaceExternalModel 的通用逻辑(仅把 dps init 视为写)。同时 TMrgSortOp::getEffects 已明确把 tmp 作为写入对象。这会导致 bufferization 分析把 tmp 当成“只读输入”,在 tensor->memref 路径上可能做出错误的 in-place/alias 决策,进而产生行为错误。

  1. P3 TRandom 的 key/counter 类型约束与 verifier/降级契约不一致(ui32 实际不可用) include/PTO/IR/PTOOps.td:3184

TRandomOp 的 6 个 key/counter 在 ODS 中定义为 AnySignlessInteger,因此 ui32 会在类型约束阶段被拒绝;但 verifier 文案和 EmitC helper 都按 i32/ui32uint32_t)语义处理。该不一致会导致前端按 unsigned 语义生成 ui32 参数时出现兼容性问题(无法通过 op 约束)。

@HecreReed HecreReed marked this pull request as ready for review April 15, 2026 06:30
@HecreReed
Copy link
Copy Markdown
Collaborator Author

/run a3

@reedhecre
Copy link
Copy Markdown

A3 板测完成(有跳过)

  • 触发方式:manual
  • 源码提交:bc22ef47225c
  • 结果汇总:OK 184 / FAIL 0 / SKIP 1
  • 日志:/home/zhongxuan/ptoas-board-monitor/runtime/logs/20260415_144004_manual_pr483.log
  • 结果 TSV:/home/zhongxuan/ptoas-board-monitor/runtime/logs/20260415_144004_manual_pr483.tsv
  • 手动指令:/run a3
  • 触发人:HecreReed
  • 触发评论:feat: align tcvt/random/mrgsort/matmulmx with pto-isa #483 (comment)

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.

2 participants