-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
misc: fix arguments for riscv_lowering #3900
misc: fix arguments for riscv_lowering #3900
Conversation
@@ -80,7 +80,7 @@ def match_and_rewrite(self, op: func.CallOp, rewriter: PatternRewriter) -> None: | |||
new_result_types = list(a_regs(op.results)) | |||
new_op = riscv_func.CallOp(op.callee, moved_operands, new_result_types) | |||
move_result_ops, moved_results = move_to_unallocated_regs( | |||
new_op.results, operand_types | |||
new_op.results, new_result_types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like a bug? maybe our funcs were void so far? could you please add a test case for this?
let us know if you need a hand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Chris, how would you recommend I test this? It looks like I need to modify line 6 here, but I am not sure how to exercise this change and don't remember how I encountered the bug :(
What do you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just modifying it to be (i32, f32) -> (i64, f64)
should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean doing this:
func.func @main() {
%0, %1 = "test.op"() : () -> (i32, f32)
%2, %3 = func.call @foo(%0, %1) : (i32, f32) -> (i64, f64)
func. return
}
Which I suppose will require changing foo()
too to accept an f32
type:
func.func @foo(%arg0 : i32, %arg1 : i32) -> (i32, i32) {
%res0, %res1 = "test.op"(%arg0, %arg1) : (i32, i32) -> (i32, i32)
func.return %res0, %res1 : i32, i32
}
Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah these would all have to be consistent within the file but that's the essence of the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for splitting the PRs btw
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3900 +/- ##
==========================================
+ Coverage 91.26% 91.29% +0.03%
==========================================
Files 466 466
Lines 57960 58012 +52
Branches 5567 5566 -1
==========================================
+ Hits 52897 52962 +65
+ Misses 3639 3622 -17
- Partials 1424 1428 +4 ☔ View full report in Codecov by Sentry. |
@@ -77,10 +77,13 @@ def match_and_rewrite(self, op: func.CallOp, rewriter: PatternRewriter) -> None: | |||
move_operand_ops, moved_operands = move_to_a_regs( | |||
register_operands, operand_types | |||
) | |||
|
|||
new_result_value_types = [result.type for result in op.results] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a bug here that makes it raise an error for floating point registers:
xdsl/xdsl/backend/riscv/lowering/utils.py
Lines 57 to 67 in 6f2c376
elif isinstance(rd, riscv.FloatRegisterType): | |
match value_type: | |
case builtin.Float64Type(): | |
mv_op = riscv.FMvDOp(value, rd=rd) | |
case builtin.Float32Type(): | |
mv_op = riscv.FMVOp(value, rd=rd) | |
case _: | |
raise NotImplementedError( | |
f"Move operation for float register containing value of type {value.type} is not implemented" | |
) | |
return mv_op, mv_op.rd |
value_type
in the original code is neither of the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a FloatRegisterType.
@superlopuh Fixing some potential bugs I encountered while using xDSL as a backend