fix(ptodsl): preserve signless arithmetic operand types - #1408
Conversation
d569134 to
ef3033f
Compare
BackgroundThis PR addresses the first PTOAS-related compilation failure observed while compiling TileLang's VMI (num_tokens + 3) // 4Here, Observed failureBefore this change, PTOAS could emit malformed %c3_i32 = arith.constant 3 : i32
%5 = builtin.unrealized_conversion_cast %c3_i32 : si32 to i32The SSA value %1 = builtin.unrealized_conversion_cast %arg2 : si32 to i32
%4 = builtin.unrealized_conversion_cast %1 : si32 to i32The second cast again declares an Root causeMLIR arithmetic operations use signless integer types ( The important issue is therefore not that FixThe fix is intentionally limited to the arithmetic adaptation boundary in:
Specifically:
This keeps the change at the exact boundary where Regression coverageThe new regression test is:
It covers a signed runtime argument, literal arithmetic, floor division, index arithmetic, and a signed store. The test also checks that no malformed literal conversion of the following form is generated: %c\\w+ = builtin\\.unrealized_conversion_cast .*: si32 to i32Validation performed with PTOAS rebuilt from this PR branch:
All of the above compilation and frontend regression checks pass with the patched PTOAS. Scope relative to the other failure classThe separate second failure class concerns a non-32-byte GM-to-UB MTE destination row stride in packed UE8M0 Hardware launch/numerical validation was not possible in this environment because NPU runtime initialization fails ( |
ef3033f to
dbbfe84
Compare
|
这个 PR 解决的是一个 PTOAS 编译器在处理整数运算时的类型对不上问题。 通俗地说,TileLang 的 (num_tokens + 3) // 4其中:
旧版本 PTOAS 在处理这类表达式时,会把运行时参数当成一种“带符号整数”(例如 %c3_i32 = arith.constant 3 : i32
%x = builtin.unrealized_conversion_cast %c3_i32 : si32 to i32这里 这个 PR 的解决方式 修复集中在 PTOAS 的标量类型适配逻辑:
所以,这个 PR 不是把所有整数都粗暴替换成 修复后,原先 |
3ee6e24 to
b34e47a
Compare
Zhendong404
left a comment
There was a problem hiding this comment.
Overall the direction is right: keeping arith operands signless from materialization through the op removes the "si32 identity + i32 value" mismatch that broke topk_gate. A few issues inline — the main one is a silent unsigned semantics regression introduced by the signless-reconcile rule.
| # manufacturing a signed literal cast that can be reused with stale | ||
| # type metadata by the tracer. | ||
| if _integer_signedness(lhs_type) == "signless" or _integer_signedness(rhs_type) == "signless": | ||
| target_type = _signless_integer_type(target_type) |
There was a problem hiding this comment.
This rule silently changes unsigned semantics. Since _materialize_runtime_literal now anchors literals as signless, any uiN runtime value combined with a literal lands here and the result type becomes signless iN. Downstream op selection reads signedness from the reconciled type (_runtime_scalar_ops.py: emit_runtime_binary_op/emit_runtime_compare/emit_runtime_min/emit_runtime_max), where signless defaults to signed — so e.g. (n_ui32 + 3) // 4 now lowers to floordivsi and n_ui32 + 3 < k to slt instead of divui/ult. For values >= 2^31 this is a silent numeric wrong-result, not a compile error. Suggest preserving the non-signless side's authored signedness on the result (compute in the signless domain, restore the uiN/siN identity afterwards), or at minimum capture the operand signedness before reconciliation for op selection. Either way, please add a ui32 regression test.
There was a problem hiding this comment.
Fixed in 0331b1e. Reconciliation now returns the authored integer type separately from the signless arithmetic operands. Runtime op selection uses that authored type, so ui32 with a literal lowers to divui/remui and unsigned comparison predicates, while MLIR arithmetic operands remain signless. Results are restored to the authored uiN/siN type. A dedicated ui32 regression now covers add, floor division, remainder, comparison, min, and max.
| # Arithmetic operands are signless. When a signed/unsigned runtime | ||
| # value is combined with a literal (which is intentionally emitted as | ||
| # signless), keep the common operand type signless through the op and | ||
| # restore the authored signedness only on the result. This avoids |
There was a problem hiding this comment.
This comment says "restore the authored signedness only on the result", but the implementation does not actually restore the authored signedness: emit_runtime_binary_op restores to lhs.type, which is the reconciled signless type at this point, not the authored si32/ui32. Either restore the authored type (preferred — see my other comment on the unsigned case) or reword the comment so it does not mislead future maintenance.
There was a problem hiding this comment.
Fixed in 0331b1e. The result is now restored using the authored type captured before signless reconciliation, rather than the reconciled lhs.type. The comment is accurate with the updated implementation.
| # constants instead of constructing a signed value and immediately | ||
| # stripping it again (the latter used to print malformed ``siN to iN`` | ||
| # casts for expressions such as ``num_tokens + 3``). | ||
| signless_type = _signless_integer_type(anchor_type) |
There was a problem hiding this comment.
int(value) narrows the accepted literal forms: the previous path went through _materialize_integer_literal → _parse_integer_value, which also accepts integer strings (e.g. "0x10"). If string literals are still part of the surface, this is a small behavior regression; suggest raw = _parse_integer_value(value, target_type=anchor_type) here to keep parity.
There was a problem hiding this comment.
Fixed in 0331b1e. _materialize_runtime_literal now uses _parse_integer_value(value, target_type=anchor_type) before creating the signless constant, preserving integer strings such as "0x10".
| # the malformed form was `... %c3_i32 : si32 to i32`. | ||
| import re | ||
|
|
||
| assert not re.search(r"%c\w+ = builtin\.unrealized_conversion_cast .*: si32 to i32", text) |
There was a problem hiding this comment.
Test coverage suggestions: (1) add an unsigned (pto.ui32) case covering add/floordiv/mod/compare/min/max — that is where the reconcile rule can silently flip op selection to signed (divui→floordivsi, ult→slt); (2) this negative regex is tied to one specific malformed shape (%c* name, si32→i32); a more robust check would catch any unrealized_conversion_cast whose declared source type mismatches the defining constant, or at least generalize to si\d+ to i\d+; (3) import re belongs at module top; (4) assert "si32" in text / "i32" in text are nearly tautological and can be dropped or tightened.
There was a problem hiding this comment.
The unsigned regression was added in 0331b1e and checks divui, remui, cmpi ult, maxui, and minui. The malformed-cast assertion remains focused on the exact siN-to-iN constant-cast shape that triggered this PR; authored-type propagation now covers the broader semantic issue. The top-level import re and assertion cleanup are minor follow-ups, and can be included in a subsequent cleanup if preferred.
Fix runtime integer arithmetic lowering after 5eb87c2. Integer literals are emitted signless, and mixed signed/signless operands are reconciled in signless arithmetic types. Explicit signed/unsigned scalar and VMI storage semantics remain unchanged. Regression: PTODSL jit compile suite and topk_gate E=256/384/512/72 K=8/9/8/6 num_sms=2.