fix: align thistogram HistByte support with PTO-ISA#697
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the pto.thistogram operation by replacing the isMSB boolean attribute with a byte integer attribute and adding support for ui32 source types. These changes include updated documentation, IR definitions, and verification logic to handle specific filtering behaviors and layout requirements for different source data types. The review feedback suggests improving the descriptiveness of verification error messages by including expected row counts and removing a redundant variable initialization in the EmitC lowering logic.
| int64_t expectedIdxRows = 1; | ||
| if (byte == 1) | ||
| expectedIdxRows = 2; | ||
| else if (byte == 0) | ||
| expectedIdxRows = 3; | ||
| if (!hasCompatibleKnownExtent(idxShape[0], expectedIdxRows) || | ||
| !hasCompatibleKnownExtent(idxValid[0], expectedIdxRows)) | ||
| return emitOpError("expects idx rows/valid rows to match the byte-selected filter depth when src element type is ui32"); |
There was a problem hiding this comment.
The error message could be more descriptive by including the expected number of rows. This helps developers quickly identify and fix shape mismatches in their IR.
int64_t expectedIdxRows = 1;
if (byte == 1)
expectedIdxRows = 2;
else if (byte == 0)
expectedIdxRows = 3;
if (!hasCompatibleKnownExtent(idxShape[0], expectedIdxRows) ||
!hasCompatibleKnownExtent(idxValid[0], expectedIdxRows))
return emitOpError("expects idx rows/valid rows to be ")
<< expectedIdxRows
<< " to match the byte-selected filter depth when src element "
"type is ui32";|
|
||
| auto templateArgs = rewriter.getArrayAttr({emitc::OpaqueAttr::get( | ||
| ctx, op.getIsMSB() ? "HistByte::BYTE_1" : "HistByte::BYTE_0")}); | ||
| StringRef histByte = "HistByte::BYTE_1"; |
There was a problem hiding this comment.
The initialization of histByte is redundant as it is unconditionally assigned in the following switch statement. Removing it improves code clarity and adheres to common C++ best practices regarding avoiding unnecessary initializations.
| StringRef histByte = "HistByte::BYTE_1"; | |
| StringRef histByte; |
Codex Review该评论由 review 机器人自动更新。
SummaryThe PR preserves old textual Findings
The detailed |
2 participants
Summary
pto.thistogramwith currentPTO-ISAHistBytesemantics instead of the older bool-style lowering from fix: use HistByte template args for thistogram emitc #634isMSBwithbyte : i32in PTO IR and lowerbyte=0..3toHistByte::BYTE_0..BYTE_3THistogramconstraints for bothui16andui32sourcesWhy
PTO-ISAnow exposes fourHistBytemodes, but they are not uniformly valid for all source dtypes:ui16: onlyBYTE_0/BYTE_1ui32:BYTE_0/BYTE_1/BYTE_2/BYTE_3PR #634 only converted the EmitC lowering from
true/falsetoBYTE_1/BYTE_0, which still lagged the current upstream interface.Changes
pto.thistogramattr fromisMSB: BoolAttrtobyte: I32Attrwith default1src: ui16 | ui32byte in [0, 3]ui16only allowsbyte=0/1ui16vsui32specificidxlayout / shape constraintsbyte=0..3toHistByte::BYTE_0..BYTE_3PTO_IR_manual.mdui32lowering coverage and invalid-byte verifier coverageValidation
cmake -G Ninja -S /Users/laoda/pto/PTOAS/_pr634_histbyte_fix -B /tmp/ptoas-pr634-build ...ninja -C /tmp/ptoas-pr634-build ptoasptoas --pto-arch=a5 test/lit/pto/thistogram_emitc.pto | FileCheck ...ptoas --pto-arch=a5 test/lit/pto/thistogram_emitc_u32.pto | FileCheck ...not ptoas --pto-arch=a5 test/lit/pto/thistogram_verify_invalid_byte_a5.pto | FileCheck ...not ptoas --pto-arch=a5 test/lit/pto/thistogram_verify_invalid_ui16_byte_a5.pto | FileCheck ...