[mypyc] Fix signed integer comparison #9163
Conversation
mypyc/irbuild/ll_builder.py
Outdated
| lhs_short = self.add(Truncate(lhs, short_int_rprimitive, | ||
| c_pyssize_t_rprimitive, line)) | ||
| rhs_short = self.add(Truncate(rhs, short_int_rprimitive, | ||
| c_pyssize_t_rprimitive, line)) |
There was a problem hiding this comment.
This is not very principled. The truncate op is redundant, and also confusingly the sizes of source and target types are the same, so there's no truncation actually going on.
Instead, we should generate the correct C code for short int comparison (add the cast to a signed integer type). Maybe you can look at the type attribute of BinaryIntOp to determine whether a signed comparison is needed when emitting C.
There was a problem hiding this comment.
Yeah, the truncate also looks weird to me. So when we emit a BinaryIntOp, we check if both operands are short_int(and probably check if the op is not EQ or NEQ as well since they do not require casts to have the correct result) to cast them into py_ssize_t
There was a problem hiding this comment.
More generally, also plain tagged integers need to use a cast since they are signed.
Probably the cleanest way to check the operand types would be to update every Value to contain the result type, but I'm not sure if this is necessary.
|
Note that there's a merge conflict. |
| @@ -245,6 +247,10 @@ def test_dict_contains(self) -> None: | |||
| 'in', self.b, self.o, self.d, | |||
| """cpy_r_r0 = PyDict_Contains(cpy_r_d, cpy_r_o);""") | |||
|
|
|||
| def test_binary_int_op(self) -> None: | |||
| self.assert_emit(BinaryIntOp(bool_rprimitive, self.s1, self.s2, BinaryIntOp.SLT, 1), | |||
| """cpy_r_r0 = (Py_ssize_t)cpy_r_s1 < (Py_ssize_t)cpy_r_s2;""") | |||
There was a problem hiding this comment.
Also test unsigned comparison with int64 and int32. These need a cast to the corresponding unsigned type (uint64_t or uint32_t, respectively). Finally, test that unsigned comparison with short_int_rprimitive and signed comparison with int32 or int64 doesn't use a cast.
There was a problem hiding this comment.
Thanks! This actually catches a bug in ops.py that constant's definition are overlapped.
358522e generates inline comparison between short ints, explicit conversion to signed is missing, though, causing negative cases to fail.
This PR add explicit type casts(although the name truncate here is a little bit misleading).
This PR will fix microbenchmark
int_list.