Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds MLIR conversion support for extract and shuffle operations in the Wave compiler. The changes enable these operations to be properly converted to the Wave MLIR dialect by handling their specific attribute requirements.
Changes:
- Added MLIR conversion logic for
ExtractandShuffleoperations with special attribute handling - Added imports for
ExtractandShuffleOpoperations - Added a new test case demonstrating the conversion of a sum kernel that uses extract and shuffle operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wave_lang/kernel/wave/mlir_converter/water_emitter.py | Adds Extract and Shuffle operation imports, registers ShuffleOp in the operation mapping, and implements custom MLIR conversion logic for both operations |
| lit_tests/kernel/wave/mlir_converter.py | Adds a new test case mlir_converter_sum that verifies correct MLIR generation for a kernel using extract and shuffle operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result_type, *mlir_operands, offset, size, stride | ||
| ) | ||
| elif isinstance(node, Extract): | ||
| assert len(node.offset) == 1 |
There was a problem hiding this comment.
The assertion lacks a descriptive message explaining why the offset length must be 1. Consider adding a message like 'Extract operation requires exactly one offset value' to help with debugging.
| assert len(node.offset) == 1 | |
| assert len(node.offset) == 1, "Extract operation requires exactly one offset value" |
| offset = ir.IntegerAttr.get( | ||
| ir.IntegerType.get_signless(32), node.offset | ||
| ) | ||
| width = ir.IntegerAttr.get( | ||
| ir.IntegerType.get_signless(32), node.width | ||
| ) |
There was a problem hiding this comment.
The integer type creation ir.IntegerType.get_signless(32) is duplicated. Consider extracting it to a variable like i32_type to improve maintainability and reduce redundancy.
| offset = ir.IntegerAttr.get( | |
| ir.IntegerType.get_signless(32), node.offset | |
| ) | |
| width = ir.IntegerAttr.get( | |
| ir.IntegerType.get_signless(32), node.width | |
| ) | |
| i32_type = ir.IntegerType.get_signless(32) | |
| offset = ir.IntegerAttr.get(i32_type, node.offset) | |
| width = ir.IntegerAttr.get(i32_type, node.width) |
77495e8 to
bf87e73
Compare
Both require special-casing to convert attributes. We cannot yet lower this further because extract and shuffle don't have index sequences, neither they need to after lowering, but we can't detect that due to Signed-off-by: Alex Zinenko <git@ozinenko.com>
3f695fb to
a2fc873
Compare
Both require special-casing to convert attributes. We cannot yet lower this further because extract and shuffle don't have index sequences, neither they need to after lowering, but we can't detect that due to Signed-off-by: Alex Zinenko <git@ozinenko.com> Signed-off-by: Harsh Menon <harsh.menon@amd.com>
Both require special-casing to convert attributes.
We cannot yet lower this further because extract and shuffle don't have index sequences, neither they need to after lowering, but we can't detect that due to