Skip to content
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.

Sign up for GitHub

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

[mypyc] Fix signed integer comparison #9163

Merged
merged 5 commits into from
Jul 20, 2020
Merged

[mypyc] Fix signed integer comparison #9163

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa4995f
signed compare
TH3CHARLie Jul 17, 2020
2c4ef1f
Merge branch 'master' into sign-compare
TH3CHARLie Jul 17, 2020
b8dab9f
add signed comparison op variant and modify emit
TH3CHARLie Jul 17, 2020
198f36d
unit test and different suffix in pretty IR between unsigned/signed ops
TH3CHARLie Jul 17, 2020
a3a5924
test signed/unsigned comparison on tagged/int32/int64
TH3CHARLie Jul 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions mypyc/irbuild/ll_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from mypyc.ir.rtypes import (
RType, RUnion, RInstance, optional_value_type, int_rprimitive, float_rprimitive,
bool_rprimitive, list_rprimitive, str_rprimitive, is_none_rprimitive, object_rprimitive,
c_pyssize_t_rprimitive, is_short_int_rprimitive
c_pyssize_t_rprimitive, is_short_int_rprimitive, short_int_rprimitive
)
from mypyc.ir.func_ir import FuncDecl, FuncSignature
from mypyc.ir.class_ir import ClassIR, all_concrete_classes
Expand Down Expand Up @@ -588,7 +588,17 @@ def compare_tagged(self, lhs: Value, rhs: Value, op: str, line: int) -> Value:
branch.negated = False
self.add(branch)
self.activate_block(short_int_block)
eq = self.binary_int_op(bool_rprimitive, lhs, rhs, op_type, line)
# for now equal op, explicitly convert tagged(unsigned) to signed to include negative
# situtations
if op not in ("==", "!="):
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))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else:
lhs_short = lhs
rhs_short = rhs
eq = self.binary_int_op(bool_rprimitive, lhs_short, rhs_short, op_type, line)
self.add(Assign(result, eq, line))
self.goto(out)
self.activate_block(int_block)
Expand Down
148 changes: 80 additions & 68 deletions mypyc/test-data/analysis.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,13 +372,17 @@ def f(a):
r1, r2, r3 :: native_int
r4 :: bool
r5, r6, r7 :: native_int
r8, r9, r10, r11, r12 :: bool
r13, r14, r15 :: native_int
r16 :: bool
r17, r18, r19 :: native_int
r20, r21, r22, r23 :: bool
r8, r9 :: bool
r10, r11 :: native_int
r12, r13, r14 :: bool
r15, r16, r17 :: native_int
r18 :: bool
r19, r20, r21 :: native_int
r22, r23 :: bool
r24, r25 :: native_int
r26, r27 :: bool
y, x :: int
r24 :: None
r28 :: None
L0:
L1:
r1 = 1
Expand All @@ -392,86 +396,94 @@ L1:
r9 = r4 & r8
if r9 goto L2 else goto L3 :: bool
L2:
r10 = a < a
r0 = r10
r10 = truncate a: short_int to native_int
r11 = truncate a: short_int to native_int
r12 = r10 < r11
r0 = r12
goto L4
L3:
r11 = CPyTagged_IsLt_(a, a)
r0 = r11
r13 = CPyTagged_IsLt_(a, a)
r0 = r13
L4:
if r0 goto L5 else goto L12 :: bool
L5:
L6:
r13 = 1
r14 = a & r13
r15 = 0
r16 = r14 == r15
r17 = 1
r18 = a & r17
r19 = 0
r20 = r18 == r19
r21 = r16 & r20
if r21 goto L7 else goto L8 :: bool
r15 = 1
r16 = a & r15
r17 = 0
r18 = r16 == r17
r19 = 1
r20 = a & r19
r21 = 0
r22 = r20 == r21
r23 = r18 & r22
if r23 goto L7 else goto L8 :: bool
L7:
r22 = a < a
r12 = r22
r24 = truncate a: short_int to native_int
r25 = truncate a: short_int to native_int
r26 = r24 < r25
r14 = r26
goto L9
L8:
r23 = CPyTagged_IsLt_(a, a)
r12 = r23
r27 = CPyTagged_IsLt_(a, a)
r14 = r27
L9:
if r12 goto L10 else goto L11 :: bool
if r14 goto L10 else goto L11 :: bool
L10:
y = a
goto L6
L11:
x = a
goto L1
L12:
r24 = None
return r24
r28 = None
return r28
(0, 0) {a} {a}
(1, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 3) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 4) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 5) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 6) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 7) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 8) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 9) {a, r0, r12, x, y} {a, r0, r12, x, y}
(2, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(2, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(2, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(3, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(3, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(3, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(4, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(5, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 3) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 4) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 5) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 6) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 7) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 8) {a, r0, r12, x, y} {a, r0, r12, x, y}
(6, 9) {a, r0, r12, x, y} {a, r0, r12, x, y}
(7, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(7, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(7, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(8, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(8, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(8, 2) {a, r0, r12, x, y} {a, r0, r12, x, y}
(9, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(10, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(10, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(11, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(11, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(12, 0) {a, r0, r12, x, y} {a, r0, r12, x, y}
(12, 1) {a, r0, r12, x, y} {a, r0, r12, x, y}
(1, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 3) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 4) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 5) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 6) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 7) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 8) {a, r0, r14, x, y} {a, r0, r14, x, y}
(1, 9) {a, r0, r14, x, y} {a, r0, r14, x, y}
(2, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(2, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(2, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(2, 3) {a, r0, r14, x, y} {a, r0, r14, x, y}
(2, 4) {a, r0, r14, x, y} {a, r0, r14, x, y}
(3, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(3, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(3, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(4, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(5, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 3) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 4) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 5) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 6) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 7) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 8) {a, r0, r14, x, y} {a, r0, r14, x, y}
(6, 9) {a, r0, r14, x, y} {a, r0, r14, x, y}
(7, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(7, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(7, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(7, 3) {a, r0, r14, x, y} {a, r0, r14, x, y}
(7, 4) {a, r0, r14, x, y} {a, r0, r14, x, y}
(8, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(8, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(8, 2) {a, r0, r14, x, y} {a, r0, r14, x, y}
(9, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(10, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(10, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(11, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(11, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}
(12, 0) {a, r0, r14, x, y} {a, r0, r14, x, y}
(12, 1) {a, r0, r14, x, y} {a, r0, r14, x, y}

[case testTrivial_BorrowedArgument]
def f(a: int, b: int) -> int:
Expand Down
48 changes: 26 additions & 22 deletions mypyc/test-data/exceptions.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,13 @@ def sum(a, l):
r3, r4, r5 :: native_int
r6 :: bool
r7, r8, r9 :: native_int
r10, r11, r12, r13 :: bool
r14 :: object
r15, r16 :: int
r17 :: short_int
r18, r19 :: int
r10, r11 :: bool
r12, r13 :: native_int
r14, r15 :: bool
r16 :: object
r17, r18 :: int
r19 :: short_int
r20, r21 :: int
L0:
r0 = 0
sum = r0
Expand All @@ -156,36 +158,38 @@ L1:
r11 = r6 & r10
if r11 goto L2 else goto L3 :: bool
L2:
r12 = i < l
r2 = r12
r12 = truncate i: short_int to native_int
r13 = truncate l: short_int to native_int
r14 = r12 < r13
r2 = r14
goto L4
L3:
r13 = CPyTagged_IsLt_(i, l)
r2 = r13
r15 = CPyTagged_IsLt_(i, l)
r2 = r15
L4:
if r2 goto L5 else goto L10 :: bool
L5:
r14 = CPyList_GetItem(a, i)
if is_error(r14) goto L11 (error at sum:6) else goto L6
r16 = CPyList_GetItem(a, i)
if is_error(r16) goto L11 (error at sum:6) else goto L6
L6:
r15 = unbox(int, r14)
dec_ref r14
if is_error(r15) goto L11 (error at sum:6) else goto L7
r17 = unbox(int, r16)
dec_ref r16
if is_error(r17) goto L11 (error at sum:6) else goto L7
L7:
r16 = CPyTagged_Add(sum, r15)
r18 = CPyTagged_Add(sum, r17)
dec_ref sum :: int
dec_ref r15 :: int
sum = r16
r17 = 1
r18 = CPyTagged_Add(i, r17)
dec_ref r17 :: int
sum = r18
r19 = 1
r20 = CPyTagged_Add(i, r19)
dec_ref i :: int
i = r18
i = r20
goto L1
L8:
return sum
L9:
r19 = <error> :: int
return r19
r21 = <error> :: int
return r21
L10:
dec_ref i :: int
goto L8
Expand Down
Loading