ImportVerilog: fix signedness propagation in ConversionExpression lowering#10235
ImportVerilog: fix signedness propagation in ConversionExpression lowering#10235kaituo-crypto wants to merge 1 commit into
Conversation
|
Closing this PR for now, as my current analysis is not solid enough to justify this fix. The semantics involved here are more subtle than I initially thought, and seem to depend not only on the IEEE SystemVerilog rules but also on how the slang frontend models and propagates them into CIRCT. I need to re-check both before proposing a correct fix. |
|
Fixed [#10234 ] |
|
@kaituo-crypto could you take a look at the CI failures? It looks to me like a missing test annotation and a formatting issue |
TaoBi22
left a comment
TaoBi22
left a comment
There was a problem hiding this comment.
Thanks @kaituo-crypto, just a few comments below but mostly looks good
| // MOORE-NOT: moore.sext | ||
| // MOORE: %[[UNSIGNED_READ:.+]] = moore.read %unsigned_src : <l8> | ||
| // MOORE: %[[UNSIGNED_ZEXT:.+]] = moore.zext %[[UNSIGNED_READ]] : l8 -> l9 | ||
| // MOORE: %[[ADD:.+]] = moore.add %[[SIGNED_ZEXT]], %[[UNSIGNED_ZEXT]] : l9 No newline at end of file |
| module M( | ||
| input logic [7:0] in1, | ||
| input logic signed [7:0] in2, | ||
| output logic signed [8:0] mux_out | ||
| ); | ||
| logic [7:0] unsigned_src; | ||
| logic signed [7:0] signed_src; | ||
|
|
||
| always_comb signed_src = in2 >>> 1; | ||
| always_comb unsigned_src = in1 >> 1; | ||
| always_comb mux_out = signed_src + unsigned_src; | ||
| endmodule |
There was a problem hiding this comment.
Can this test case be reduced at all?
| // When materializing a conversion, the caller may need to override the | ||
| // source expression's signedness with the conversion's own semantics. | ||
| Value convertRvalueExpression(const slang::ast::Expression &expr, | ||
| Type requiredType,bool conversionIsSigned); |
There was a problem hiding this comment.
Could we just add an optional parameter to the existing convertRvalueExpression function?
2 participants
Summary
Fix ImportVerilog
ConversionExpressionlowering to use the conversion node's signedness when materializing widening conversions.Verification
Covered by
test/Conversion/ImportVerilog/conversion-signedness.sv.Related issue
See #10234 for the full analysis and reproducer.