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 thread lib/PTO/IR/PTO.cpp
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: a87d6c282e48
  • Trigger: PR 有新提交
  • Generated At: 2026-04-15T08:19:27Z
  • Previous Head SHA: f68598411682
  • Status: completed

Summary

发现 1 个 P2 兼容性问题:pto.tcvt 新操作数形态已落地,但 ptobc v0 仍按旧 2-operand 规格编码,导致 bytecode roundtrip 对新 tcvt 形态失败(当前仅在 runop 对 tcvt 样例做了绕过)。

Findings

  1. P2 ptobc v0 仍将 `pto.tcvt` 固定为 2 个操作数,无法编码新 `tmp/sat_mode` 形态 tools/ptobc/generated/ptobc_opcodes_v0.h:68

PR 将 pto.tcvt 扩展为可选 tmp(并新增 sat_mode),但 ptobc opcode 表仍把 pto.tcvt 定义为 2 操作数。test/samples/runop.sh 里新增了对 tcvt.py 的 roundtrip 绕过并注明会报 operand count mismatch for op: pto.tcvt,说明底层兼容性问题仍存在。结果是除该样例特判外,任何启用 ptobc roundtrip 的新 tcvt 用法都会失败,属于明确的兼容性回归。

@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