feat(hipsr): add hipsr.compute and hipsr.compute_yield ops - #615
Open
zpye wants to merge 7 commits into
Open
Conversation
|
Thanks for opening a PR! This project follows LLVM's incremental-development and AI-tool-use Before requesting review, please check that:
Reviewers are assigned through |
zpye
marked this pull request as ready for review
July 31, 2026 11:33
zpye
requested review from
amd-mingw,
edelaye,
fhanuman,
qianglin-amd and
wcy123
as code owners
July 31, 2026 11:33
4 tasks
L2 Accuracy Results (EP vs CPU)
Threshold: 0.01 | Run: 3518 - Commit: |
MorphiZen EP Performance Results
EPContext Export Performance
EPContext Import Performance
OGA Benchmark Results
OGA Wheel Smoke (Python benchmark_e2e.py)
Run: 3518 - Commit: |
wcy123
previously approved these changes
Jul 31, 2026
Collaborator
There was a problem hiding this comment.
LGTM
Minor observation: inputs defined as Variadic<AnyType> is overly permissive - should be Variadic<Hipsr_TensorOrDeviceMemRef> to match the design. The test case @multi_result passes an index input which would then be invalid and should be fixed.
hipsr.compute groups the operations that implement one high-level computation into a single DPS op with an isolated body region, so an ONNX op needing several tensor or shape operations does not leave intermediate values in the enclosing block. A hipsr.placeholder accepts only block arguments and constant or placeholder results as inputs, so loose intermediates would otherwise block placeholder-based output shaping. The op carries no hand-written verifier. Every rule the design proposed checking by hand already comes from a trait or interface: SizedRegion<1> and SingleBlock for the block count, SingleBlockImplicitTerminator for the terminator, RegionBranchOpInterface for both control-flow edges, DestinationStyleOpInterface for init and result tying, and IsolatedFromAbove for boundary crossings. test/lit/Dialect/Hipsr/ compute.mlir asserts the message each mechanism emits. Two traits from the design are left out on purpose. PromotableRegionOpInterface does not exist in the pinned MLIR, and the Promotable* family it resembles is Mem2Reg/SROA slot promotion rather than region flattening. ConditionallySpeculatable only feeds isPure, whose consumers are LICM and related hoisting paths; the hipsr pipeline has no loops to hoist a compute out of, and RecursiveMemoryEffects already lets dead-code elimination erase an unused one. This step adds the ops only, with no conversion-pattern users and no bufferization model, so a compute that reached one-shot bufferize would fail. That is unreachable today because -hipsr-bufferize does not exist. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
The getEntrySuccessorOperands comment restated the operand order the op description already documents and the roundtrip test already asserts, and the neighbouring PoolDomainOp implementation carries no such comment. Two negative LIT cases described the custom form's behaviour instead of the case in front of them. Both now use the one-line phrasing that pool-domain.mlir and placeholder-invalid.mlir use for the same checks. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
The placeholder-input rule was written as a subjunctive clause chain that took two readings to parse. It now states the constraint directly. The DPS paragraph opened by saying the outs inits are the destinations, which is what the term already means, so only the tying rule remains. The trait paragraph claimed isolation keeps "stock MLIR passes correct", which says little. It now uses the explicit-and-verifiable wording that HipsrPoolDomainOp.td uses for the same pair, and names AutomaticAllocationScope instead of describing it. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
HipsrComputeYieldOp.td explained that its operands are optional so the implicit terminator prints as a bare mnemonic. HipsrEmptyYieldOp.td carries the identical format string with no such note, and the builder two lines below already says the terminator is implicit. HipsrComputeOp.td noted that result types follow the body after ':' as in the other hipsr DPS ops. Those ops, HipsrCastOp.td among them, do not comment their own use of ':', so the note explained conformity rather than anything specific to this op. The optional-operand-group note stays, since it is what makes 'ins() outs()' legal. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
The remaining note said both operand lists are optional groups so an empty one prints as 'ins()'. The optional markers in the format say that, and compute.mlir covers the empty case. HipsrCastOp.td and the other DPS ops leave their formats uncommented. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
hipsr.compute_yield took Variadic<AnyType> while hipsr.compute declares Variadic<AnyRankedTensor> results, so the operand type was looser than the values it can legally carry. The parent's RegionBranchOpInterface check still reports first for every reachable input, since MLIR verifies the compute op's traits before recursing into its region, so this adds no new diagnostic. It states the contract where the op is defined. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Review on #615 noted Variadic<AnyType> inputs are looser than the design. They were also the outlier in the dialect: matmul, add, expand and cast take Hipsr_TensorOrDeviceMemRef, and placeholder takes AnyRankedTensor. Only pool_domain uses AnyType, and it groups operations rather than carrying tensor data. @multi_result forwarded an index to show a non-tensor input crossing the boundary. It now forwards a rank-0 tensor, which keeps the multi-input operand-segment coverage and adds a rank the suite did not have, and a new negative case pins the rejection of a scalar input. Made-with: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
zpye
force-pushed
the
feat/hipsr-compute-op
branch
from
July 31, 2026 13:29
8522653 to
768d132
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
hipsr.computeandhipsr.compute_yield.hipsr.computeis a DPS op whose isolated single-block region groups the operations that implement one high-level computation, so an ONNX op needing several tensor or shape operations stays a single hipsr op.Operands follow the rest of the dialect: inputs and inits take
Hipsr_TensorOrDeviceMemRef, matchingmatmul,add,expandandcast, andhipsr.compute_yieldtakes ranked tensors to match the op's results.Verification comes entirely from traits and interfaces, so there is no hand-written verifier.
DestinationStyleOpInterfaceties each tensor init to the result in the same position,RegionBranchOpInterfacechecks both control-flow edges,IsolatedFromAboverejects boundary crossings, andSingleBlockImplicitTerminatorpins the single block and its terminator.Ops only: nothing emits one yet, so no existing behavior changes.