transformations: (riscv-lower-parallel-mov) fix some lint errors#5527
transformations: (riscv-lower-parallel-mov) fix some lint errors#5527superlopuh merged 2 commits intomainfrom
Conversation
| rewriter.insert_op(op3) | ||
| return op2, op3 | ||
| ) -> tuple[SSAValue[riscv.IntRegisterType], SSAValue[riscv.IntRegisterType]]: | ||
| """Add swap using xors. returns the new SSAValues.""" |
| op1 = rewriter.insert_op(riscv.XorOp(a, b, rd=a.type)) | ||
| op2 = rewriter.insert_op(riscv.XorOp(op1, b, rd=b.type)) | ||
| op3 = rewriter.insert_op(riscv.XorOp(op1, op2, rd=a.type)) |
There was a problem hiding this comment.
insert_op returns the op, can make it a bit more compact and IMO readable
| op1 = rewriter.insert_op(riscv.XorOp(a, b, rd=a.type)) | ||
| op2 = rewriter.insert_op(riscv.XorOp(op1, b, rd=b.type)) | ||
| op3 = rewriter.insert_op(riscv.XorOp(op1, op2, rd=a.type)) | ||
| return op2.rd, op3.rd |
There was a problem hiding this comment.
might as well only return the new values
| rewriter.insert_op(op2) | ||
| rewriter.insert_op(op3) | ||
| return op2, op3 | ||
| ) -> tuple[SSAValue[riscv.IntRegisterType], SSAValue[riscv.IntRegisterType]]: |
There was a problem hiding this comment.
better to add return type annotations as a policy IMO
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5527 +/- ##
==========================================
+ Coverage 86.00% 86.02% +0.01%
==========================================
Files 391 392 +1
Lines 55762 55808 +46
Branches 6414 6418 +4
==========================================
+ Hits 47959 48009 +50
+ Misses 6269 6267 -2
+ Partials 1534 1532 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
anominos
left a comment
anominos
left a comment
There was a problem hiding this comment.
LGTM, I don't really like having a bunch of type casts and I wonder if we can use TypeGuards instead, but I think its fine for now.
| # split the current mov | ||
| cur_input = op.inputs[idx] | ||
| cur_input = srcs[idx] | ||
| cur_output = op.outputs[idx] |
There was a problem hiding this comment.
| cur_output = op.outputs[idx] | |
| cur_output = dsts[idx] |
| dst_type = src.type | ||
| # finish the split mov | ||
| # this assert is already checked at start, but is used for type checking | ||
| assert isinstance(cur_output.type, riscv.IntRegisterType) |
There was a problem hiding this comment.
We can remove this assert after above suggestion
| assert isinstance(cur_output.type, riscv.IntRegisterType) |
| name = "riscv.parallel_mov" | ||
| inputs = var_operand_def(RISCVRegisterType) | ||
| outputs = var_result_def(RISCVRegisterType) | ||
| outputs: VarOpResult[RISCVRegisterType] = var_result_def(RISCVRegisterType) |
There was a problem hiding this comment.
Does the pyright not resolve this automatically? Do we need to type annotate the other attributes as well?
There was a problem hiding this comment.
Unfortunately not: microsoft/pyright#10516. The thing is that we can assert this for results, as it doesn't make sense to modify them during rewriting, but this isn't the case for operands, as their types can change due to rewriting the operations that produced them.
There was a problem hiding this comment.
Ok, makes sense that we don't want to annotate inputs. How about free_registers?
|
@anominos happy with this? |
| inputs = var_operand_def(RISCVRegisterType) | ||
| outputs = var_result_def(RISCVRegisterType) | ||
| outputs: VarOpResult[RISCVRegisterType] = var_result_def(RISCVRegisterType) | ||
| free_registers = opt_prop_def(ArrayAttr[RISCVRegisterType]) |
There was a problem hiding this comment.
| free_registers = opt_prop_def(ArrayAttr[RISCVRegisterType]) | |
| free_registers: ArrayAttr[RISCVRegisterType] | None = opt_prop_def(ArrayAttr[RISCVRegisterType]) |
There was a problem hiding this comment.
This one is not necessary, because it's specifically TypeVars that Pyright cannot infer, the rest of it is fine for now. As Eric said, he might actually remove this functionality in the future, but I'd rather do the code change if it breaks with a future version of pyright instead.
There was a problem hiding this comment.
Ah actually there's no TypeVar there... I'm not sure why, but type inference works already for this definition, so I would still not add it until something breaks.
| name = "riscv.parallel_mov" | ||
| inputs = var_operand_def(RISCVRegisterType) | ||
| outputs = var_result_def(RISCVRegisterType) | ||
| outputs: VarOpResult[RISCVRegisterType] = var_result_def(RISCVRegisterType) |
There was a problem hiding this comment.
Ok, makes sense that we don't want to annotate inputs. How about free_registers?
|
Ok, LGTM |
2 participants
@anominos I had some weird linting errors locally that weren't on the CI, so I thought I'd lint them, and got a little carried away. This PR contains:
srcanddstused in one loop fromsrc_typeanddst_typeused in another loop. The current approach of usingsrcanddstfor both the values and the types confused pyright.src_by_dst_type, which is my preferred dictionary naming approachVarOpResultannotation to the parallel move op to remove a cast@anominos please take a look, and let me know if there are things that you don't like, I'm happy to scale it down or split things up.