[water] fixes to wave.extract #791
Open
Conversation
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
This operation normally appears after expansion, so after analyses and should not participate in them. We also don't exactly know how it would participate in them as it refers to a partially-lowered state reasoning about vectors. It shouldn't have identical tensor types before and after given its semantics, and such typing would contradict the Python equivalent. Also enforce vector-level invariants when the operation is in vector form. Signed-off-by: Alex Zinenko <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the wave.extract operation to correctly represent its role as an internal lowering operation and enforce stricter type constraints.
Changes:
- Removed analysis-related traits from
wave.extractsince it appears post-expansion - Added verification to ensure result types are 1-dimensional tensors or 1-element vectors
- Updated operation description to clarify its internal/lowering nature
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| water/include/water/Dialect/Wave/IR/WaveOps.td | Removed analysis traits, updated description to clarify internal operation status, and added common arguments syntax |
| water/lib/Dialect/Wave/IR/WaveOps.cpp | Added comprehensive verification logic for element type matching, vector/tensor dimensionality, and source-result compatibility |
| water/test/Dialect/Wave/ops-invalid.mlir | Added test cases covering invalid position rank, vector size, tensor dimensionality, unspecified types, and dimension mismatches |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1658
to
+1659
| if (!resultTensorType.getFullySpecified() || | ||
| resultTensorType.getRank() != 1) { |
There was a problem hiding this comment.
The error message at line 1660 does not distinguish between the two failure conditions (not fully specified vs. incorrect rank). Consider splitting this into two separate checks with distinct error messages to improve debugging clarity.
Suggested change
| if (!resultTensorType.getFullySpecified() || | |
| resultTensorType.getRank() != 1) { | |
| if (!resultTensorType.getFullySpecified()) { | |
| return emitOpError() | |
| << "result tensor type must be fully specified, got " | |
| << resultTensorType; | |
| } | |
| if (resultTensorType.getRank() != 1) { |
| Therefore, the result tensor is always indexed by one dimension. | ||
|
|
||
| TODO: we don't have a good way of verifying this, but the indexing symbol | ||
| symbol of the result must be the indexing symbol for which the size |
There was a problem hiding this comment.
The word 'symbol' is duplicated on line 306. It should appear only once.
Suggested change
| symbol of the result must be the indexing symbol for which the size | |
| of the result must be the indexing symbol for which the size |
This is needed for lowering after all Signed-off-by: Alex Zinenko <[email protected]>
Signed-off-by: Alex Zinenko <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This operation normally appears after expansion, so after analyses and should not participate in them. We also don't exactly know how it would participate in them as it refers to a partially-lowered state reasoning about vectors.
It shouldn't have identical tensor types before and after given its semantics, and such typing would contradict the Python equivalent.
Also enforce vector-level invariants when the operation is in vector form.